Prevent unnecessary string scanning. #16
Conversation
The internal strcat_s() function already calls strlen() on the source/destination strings to check for overflows. Instead of calling into strcat() and scanning over the string again, we can simply call memcpy() to copy the text over to the right place. Apply the same optimization to strcpy_s(). memcpy() is almost always faster than strcpy(), for the reason that testing against a counter is faster than searching for a null byte in the input sequence. This change allows the code to build with Nuxi CloudABI's C library[1]. This C library does not provide implementations for strcpy(), strcat() and strncat(), as in addition to being potentially insecure, the observation is that if you want to use these securely, you can almost always use memcpy() instead. [1] cloudlibc: https://github.com/NuxiNL/cloudlibc
| @@ -207,9 +203,11 @@ namespace boost{ namespace re_detail{ | |||
| const char *strSource | |||
| ) | |||
| { | |||
| if(std::strlen(strSource) + std::strlen(strDestination) + 1 > sizeInBytes) | |||
| std::size_t lenSource = std::strlen(strSource) + 1; | |||
| std::size_t lenDestination = std::strlen(strDestination); | |||
There was a problem hiding this comment.
lenSource and lenDestination names are confusingly similar while one includes the terminating zero and the other doesn't.
There was a problem hiding this comment.
There two ways we can resolve this. Both ways are fine by me. Be sure to let me know which one you prefer.
- Rename
lenSourceto something likelenSourceWithNull, or - Don't add the
+ 1and add it in both places where we need it.
My only concern with the second approach is that it makes the arithmetic that's going on less obvious. Right now it is quite obvious that this will never access the buffer out of bounds.
There was a problem hiding this comment.
I think renaming would be better.
By renaming it to lenSourceWithNull, it is more obvious that the length includes the trailing null byte.
|
What happened to this PR? There was immediate engagement then it was dropped for 3 years? This PR targets the wrong destination branch. Recommend splitting and resolving, or closing the PR. |
|
Manually merged to develop. |
The internal strcat_s() function already calls strlen() on the
source/destination strings to check for overflows. Instead of calling
into strcat() and scanning over the string again, we can simply call
memcpy() to copy the text over to the right place.
Apply the same optimization to strcpy_s(). memcpy() is almost always
faster than strcpy(), for the reason that testing against a counter is
faster than searching for a null byte in the input sequence.
This change allows the code to build with Nuxi CloudABI's C library[1].
This C library does not provide implementations for strcpy(), strcat()
and strncat(), as in addition to being potentially insecure, the
observation is that if you want to use these securely, you can almost
always use memcpy() instead.
[1] cloudlibc: https://github.com/NuxiNL/cloudlibc