gearman / gearmand

http://gearman.org/
Other
741 stars 138 forks source link

Fix strncpy() warning in libtest/socket.cc when compiling with gcc 8 #239

Closed esabol closed 5 years ago

esabol commented 5 years ago

This splits off one of the changes from PR #229 into a separate PR.

It fixes the following warning message when compiled with gcc 8.2.0+:

libtest/socket.cc:58:12: warning: ‘char* strncpy(char*, const char*, size_t)’
         output truncated before terminating nul copying as many bytes from a
         string as its length [-Wstringop-truncation]
         strncpy(global_socket, socket, strlen(socket));

More information: https://developers.redhat.com/blog/2018/05/24/detecting-string-truncation-with-gcc-8/

Relevant section:

Mitigating strncpy Truncation

Since it is not possible to avoid truncation by strncpy, when using other functions is not feasible, it is necessary to make sure the result of strncpy is properly NUL-terminated and the NUL must be inserted explicitly, after strncpy has returned:

char pathname[256];
strncpy (pathname, dirname, sizeof pathname);
pathname[sizeof pathname - 1] = '\0';

GCC tries to detect these uses and avoid issuing the warning when it can determine that the NUL is inserted before the array is used by a string-handling function. However, the simple approach outlined above suffers from the same problem as ignoring snprintf truncation and so, to be safe, the truncation should be detected and handled as discussed above. GCC 8 doesn’t detect the missing handling in this case but future versions might.

esabol commented 5 years ago

It took me a while to come to this realization, but using strlen(socket) for the third argument to strncpy() is clearly a bug. A harmless one, but still a bug. The relevant size for any strncpy() call is the size of the destination string, not the length (or size) of the source string.

esabol commented 5 years ago

you could use git commit --amend to reduce git log history.

Thanks for the tip. I've squashed the three commits into a single commit for the sake of cleanliness.

sni commented 5 years ago

How about this one? Could it be merged?

p-alik commented 5 years ago

It seems there is no objection to merge this pull request