use strndup/memdup instead of malloc, memcpy and null-terminate etc #12453
Conversation
bagder
changed the title
bagder
changed the title
|
@jay: I want to remove the |
It makes it possible to clone a binary chunk of data.
- bufref: use strndup - cookie: use strndup - formdata: use strndup - ftp: use strndup - gtls: use aprintf instead of malloc + strcpy * 2 - http: use strndup - mbedtls: use strndup - md4: use memdup - ntlm: use memdup - ntlm_sspi: use strndup - pingpong: use memdup - rtsp: use strndup instead of malloc, memcpy and null-terminate - sectransp: use strndup - socks_gssapi.c: use memdup - vtls: use dynbuf instead of malloc, snprintf and memcpy - vtls: use strdup instead of malloc + memcpy - wolfssh: use strndup Closes #12453
bagder
force-pushed
the
bagder/rtsp-strndup
branch
from
442f34b
to
f2089f2
Compare
| if(!buf) | ||
| return NULL; |
There was a problem hiding this comment.
I want to remove the memchr stuff again from strndup() because I think it potentially breaks several of these strndup cases.
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:
| if(!buf) | |
| return NULL; | |
| char *end; | |
| if(!buf) | |
| return NULL; | |
| end = memchr(src, '\0', length); | |
| if(end) | |
| length = end - src; |
There was a problem hiding this comment.
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.
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.
We want:
- alloc N bytes + one byte null termation
- copy N bytes (not to the nearest null byte) to the target memory
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.
- bufref: use strndup - cookie: use strndup - formdata: use strndup - ftp: use strndup - gtls: use aprintf instead of malloc + strcpy * 2 - http: use strndup - mbedtls: use strndup - md4: use memdup - ntlm: use memdup - ntlm_sspi: use strndup - pingpong: use memdup - rtsp: use strndup instead of malloc, memcpy and null-terminate - sectransp: use strndup - socks_gssapi.c: use memdup - vtls: use dynbuf instead of malloc, snprintf and memcpy - vtls: use strdup instead of malloc + memcpy - wolfssh: use strndup Closes #12453
| @@ -116,12 +117,9 @@ CURLcode Curl_bufref_memdup(struct bufref *br, const void *ptr, size_t len) | |||
| DEBUGASSERT(len <= CURL_MAX_INPUT_LENGTH); | |||
|
|
|||
| if(ptr) { | |||
| cpy = malloc(len + 1); | |||
| cpy = Curl_strndup(ptr, len); | |||
There was a problem hiding this comment.
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.
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.
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.
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.
... one would think that it has this functionality...
This happened to me: will probably affect others.
|
Curl_memdup0 ?Am 08.12.2023 um 14:05 schrieb Daniel Stenberg ***@***.***>:
@bagder commented on this pull request.
In lib/bufref.c:
@@ -116,12 +117,9 @@ CURLcode Curl_bufref_memdup(struct bufref *br, const void *ptr, size_t len)
DEBUGASSERT(len <= CURL_MAX_INPUT_LENGTH);
if(ptr) {
- cpy = malloc(len + 1);
+ cpy = Curl_strndup(ptr, len);
I've been pondering on maybe renaming this function just to make it less likely that one would think that it has this functionality...
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>
|
|
I made #12490 |
No description provided.