jmuehlner commented on code in PR #517:
URL: https://github.com/apache/guacamole-server/pull/517#discussion_r1595758588
##########
src/libguac/string.c:
##########
@@ -124,18 +124,30 @@ char* guac_strdup(const char* str) {
/* Do not attempt to duplicate if the length is somehow magically so
* obscenely large that it will not be possible to add a null terminator */
size_t length;
- if (guac_mem_ckd_add(&length, strlen(str), 1))
+ size_t length_to_copy = strnlen(str, n);
+ if (guac_mem_ckd_add(&length, length_to_copy, 1))
return NULL;
- /* Otherwise just copy to a new string in same manner as strdup() */
- void* new_str = guac_mem_alloc(length);
- if (new_str != NULL)
- memcpy(new_str, str, length);
+ /* Otherwise just copy to a new string in same manner as strndup() */
+ char* new_str = (char*)guac_mem_alloc(length);
+ if (new_str != NULL) {
+ memcpy(new_str, str, length_to_copy);
+ new_str[length_to_copy] = '\0';
Review Comment:
Looks to me like `guac_strdup()` did add a NULL terminator... it added 1 to
`strlen(str)` and then copied that many bytes over, which will include the
terminator. What am I missing?
Also, `guac_strdup()` _should add a NULL-terminator, as it's described as
behaving identically to `strdup()` (which adds a NULL-terminator), aside from
the NULL check.
##########
src/libguac/string.c:
##########
@@ -124,18 +124,30 @@ char* guac_strdup(const char* str) {
/* Do not attempt to duplicate if the length is somehow magically so
* obscenely large that it will not be possible to add a null terminator */
size_t length;
- if (guac_mem_ckd_add(&length, strlen(str), 1))
+ size_t length_to_copy = strnlen(str, n);
+ if (guac_mem_ckd_add(&length, length_to_copy, 1))
return NULL;
- /* Otherwise just copy to a new string in same manner as strdup() */
- void* new_str = guac_mem_alloc(length);
- if (new_str != NULL)
- memcpy(new_str, str, length);
+ /* Otherwise just copy to a new string in same manner as strndup() */
+ char* new_str = (char*)guac_mem_alloc(length);
+ if (new_str != NULL) {
+ memcpy(new_str, str, length_to_copy);
+ new_str[length_to_copy] = '\0';
Review Comment:
Looks to me like `guac_strdup()` did add a NULL terminator... it added 1 to
`strlen(str)` and then copied that many bytes over, which will include the
terminator. What am I missing?
Also, `guac_strdup()` _should_ add a NULL-terminator, as it's described as
behaving identically to `strdup()` (which adds a NULL-terminator), aside from
the NULL check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
