-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
use strndup/memdup instead of malloc, memcpy and null-terminate etc #12453
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -104,18 +104,14 @@ void *Curl_memdup(const void *src, size_t length) | |||||||||||||||||
* Curl_strndup(source, length) | ||||||||||||||||||
* | ||||||||||||||||||
* Copies the 'source' string to a newly allocated buffer (that is returned). | ||||||||||||||||||
* Copies not more than 'length' bytes (up to a null terminator) then adds a | ||||||||||||||||||
* null terminator. | ||||||||||||||||||
* Copies 'length' bytes then adds a null terminator. | ||||||||||||||||||
* | ||||||||||||||||||
* Returns the new pointer or NULL on failure. | ||||||||||||||||||
* | ||||||||||||||||||
***************************************************************************/ | ||||||||||||||||||
void *Curl_strndup(const char *src, size_t length) | ||||||||||||||||||
{ | ||||||||||||||||||
char *buf = memchr(src, '\0', length); | ||||||||||||||||||
if(buf) | ||||||||||||||||||
length = buf - src; | ||||||||||||||||||
buf = malloc(length + 1); | ||||||||||||||||||
char *buf = malloc(length + 1); | ||||||||||||||||||
if(!buf) | ||||||||||||||||||
return NULL; | ||||||||||||||||||
Comment on lines
115
to
116
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
How is it breaking? If we don't check if the length of src is shorter than the passed in length then the memcpy can read past the end of src. If the full passed in length needs to be allocated then it could be adjusted after the allocation but memchr would need to be used:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because nowhere in the code does it assume that it should only add the substring up to a null terminator. They all want the specified subtring allocated, with a null terminator so that it can be treated as a string. In some cases the substring might contain a null byte and then the added null terminator is of course unnecessary but that's how the code already works. If the memchr() logic remains , this use case breaks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you give me an example? I still don't get it. edit: If you mean the rest of the memory should be cleared then use calloc instead of malloc? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We want:
Not one of these cases want the copy to stop at the first null byte. See the sectransp.c, ntlm_sspi.c and mbedtls.c changes. Scanning for null bytes in there is just wasteful or at worst wrong. |
||||||||||||||||||
memcpy(buf, src, length); | ||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is wrong: the function is supposed to duplicate the given byte count, but this will stop at the first null byte.
The original code behaves more like memdup, but appends a null byte as a sentinel in case the data is effectively a string.
Maybe we should have a such a generic dual-use duplication function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, Curl_strndup() does not stop at the first null byte (any more). That is exactly what I mentioned above in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad! I've been abused by the Curl_strndup() original code and the usage semantics associated with this name.
Sorry for the noise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been pondering on maybe renaming this function just to make it less likely that one would think that it has this functionality...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This happened to me: will probably affect others.