Skip to content

Conversation

zvpunry
Copy link
Contributor

@zvpunry zvpunry commented Jan 12, 2020

strncpy() may not terminate the dest if it is smaller than the given
length.

This has been fixed to silence a compiler warning. It is just library
test code, nothing that could cause problems in gearman.

strncpy() may not terminate the dest if it is smaller than the given
length.

This has been fixed to silence a compiler warning. It is just library
test code, nothing that could cause problems in gearman.
@esabol
Copy link
Member

esabol commented Jan 12, 2020

I have confirmed that the build fails when compiled with gcc 9.2.1 + libmemached-dev + memcached installed on Ubuntu 18.04. With this change, the build succeeds.

For future reference, our usual fix of deliberately NUL-terminating the string right after the strncpy() call (something along the lines of memcached_binary_path[sizeof memcached_binary_path -1]= '\0';) doesn't work with gcc 9.2.1, even though it should. Something about the source string being arg_buffer.str().c_str() causes the check that gcc-9's -Wstringop-truncation does to fail. For example, this silences the warning:

      const char *foo = arg_buffer.str().c_str();
      strncpy(memcached_binary_path, foo, sizeof memcached_binary_path);
      memcached_binary_path[sizeof memcached_binary_path -1]= '\0';

but this doesn't:

      strncpy(memcached_binary_path, arg_buffer.str().c_str(), sizeof memcached_binary_path);
      memcached_binary_path[sizeof memcached_binary_path -1]= '\0';

Weird! Apparently, this is a bug in gcc 9.x according to https://stackoverflow.com/questions/56253996/why-does-gcc-9-1-0-sometimes-complain-about-this-use-of-strncpy/56255379 and is tracked at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88780. Sadly not much progress on fixing that though, so we need to work around it. I prefer @zvpunry's solution here of just making the string one char bigger.

@SpamapS SpamapS merged commit 0cebb2a into gearman:master Feb 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants