GCC8/9 produces warning on strncpy

[TrainzLuvr]

Got this error:

arm-linux-gnueabihf-g++ -c -g -O3 -march=armv6 -marm -Wall -Werror -Wno-unknown-pragmas -MD -MP -fno-stack-protector -D_GNU_SOURCE -std=c++0x -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__USE_LIBSTDCPP__ -I./ -I/home/user/Development/openmrn/src/ -I/home/user/Development/openmrn/include -MD -MF SimpleNodeInfo.d /home/user/Development/openmrn/src/openlcb/SimpleNodeInfo.cxx -o SimpleNodeInfo.o
/home/user/Development/openmrn/src/openlcb/SimpleNodeInfo.cxx: In function ‘void openlcb::init_snip_user_file(int, const char*, const char*)’:
/home/user/Development/openmrn/src/openlcb/SimpleNodeInfo.cxx:70:12: error: ‘char* strncpy(char*, const char*, size_t)’ specified bound 63 equals destination size [-Werror=stringop-truncation]
     strncpy(data.user_name, user_name, sizeof(data.user_name));
     ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/user/Development/openmrn/src/openlcb/SimpleNodeInfo.cxx:71:12: error: ‘char* strncpy(char*, const char*, size_t)’ specified bound 64 equals destination size [-Werror=stringop-truncation]
     strncpy(data.user_description, user_description,
     ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             sizeof(data.user_description));
             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[balazsracz]

This has nothing to do with socketcan. GCC8/9 produces a warning that previous versions of GCC did not. Unfortunately in this case the warning is a false positive, and expresses the opinion of GCC authors that all invocations of strncpy are wrong. I find that to be a pretty strong statement. In the place where this warning is emitted we want exactly and very specifically the semantics that strncpy provides.

anyway, we have to fix this.

Alex's master branch has a workaround btw, but i believe that introduces bugs unlike the current code which is correct.

[atanisoft]

@balazsracz Alex's workaround can lead to copying memory beyond the size of user_name or user_description. Perhaps we can move these strings to a std::string and use standard assignment instead of strncpy?

[atanisoft]

Also interestingly I do not see this error when building with GCC 8.2.0 on the esp32.

[atanisoft]

@balazsracz a possible workaround: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87028#c6, we may need to also ensure truncation at 63/64 characters.

[TrainzLuvr]

Was this strncpy here before, because I did not change my compilation environment, and previous builds produced no such warnings or errors?

[atanisoft]

@TrainzLuvr Yes, it has been there for a long time now.

[TrainzLuvr]

Then I don't understand why I only got this now. I was compiling the Hub for rPi and few days ago it went clean. Now with the latest pull from master, I can't get it compiled and this error comes up..

[balazsracz]

You were compiling from Alex's master which has a workaround. If you were to compile from openmrn master before the socketcan PR, you will get the same error. You have to use the same compiler though.

[TrainzLuvr]

I did not pull Alex's branch though, instead I added/altered the files he used to my OpenMRN branch of master locally. Either way, that workaround must have been included in the changes he made to those specific files.

Looking around the web they had some patch for this issue but I don't see any other version of gcc available beyond 8.3.0, at least not on my distro.

[TrainzLuvr]

I see it now, he used memcpy instead of strncpy. Although the advice I saw on the web regarding this issue was to continue using strncpy when strings are involved.

Since this change was excluded from code merge into master, what options are left...manually compile a newer gcc?

[atanisoft]

you can try:

strncpy(data.user_name, user_name, sizeof(data.user_name) - 1);

and see if that fixes it, at least as a temporary.

[balazsracz]

We are committed to fixing this problem. This thread is about figuring out what the right fix is. I'm open to suggestions.

• Alex fixed by switching to memcpy, which is not the right fix, because it will copy data behind these strings into the response buffer that goes out on the wire.
• We can add -Wno-stringop-truncation to the compiler flags.
• We can write our own strncpy that GCC does not know about.
• Apparently we could sacrifice the last character; if we set it to always 0, GCC will stop complaining. This does not strictly speaking match our protocol definition but might be acceptable. It is safer to operate the protocol with one fewer characters max.

I'm not excited about the following solutions because they are brittle:

• adding parentheses

• pragma GCC i am annoyed (could cause warnings on other compilers)

[atanisoft]

Alex fixed by switching to memcpy, which is not the right fix, because it will copy data behind these strings into the response buffer that goes out on the wire.

This concern can be mitigated by using something like:

strncpy(data.user_name, user_name, std::min(sizeof(data.user_name), strlen(user_name)));

[TrainzLuvr]

Reading some of the posts from people who have run into this issue, and implemented their own strncpy to guarantee string is nul-terminated, they say that GCC logic isn't always sure about it and throws a false positive anyway.

IMHO, I would go with the compiler flag and defer the issue until upstream GCC 8/9 fix is deployed far and wide.

[bakerstu]

I’m not in favor of a compiler flag fix unless it is localized inside the file with a #pragma push/pop.

On Sep 3, 2020, at 12:46 PM, TrainzLuvr notifications@github.com wrote:

Reading some of the posts from people who have run into this issue, and implemented their own strncpy to guarantee string is nul-terminated, they say that GCC logic isn't always sure about it and throws a false positive anyway.

IMHO, I would go with the compiler flag and defer the issue until upstream GCC 8/9 fix is deployed far and wide.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bakerstu/openmrn/issues/424#issuecomment-686649322, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARMLDRQMU7X32GCJDR2T4LSD7I57ANCNFSM4QUB3IHA.

[TrainzLuvr]

There are 5 instances of use of strncpy in OpenMRN code. Considering the total number of lines of code, to me it seems insignificant.

Out of those 5 instances, 4 are straight strncpy use, but in one at https://github.com/bakerstu/openmrn/blob/aad519cdd33bd78cc36155e2c010648d5ead140d/src/utils/EntryModel.hxx#L123 someone had already conveniently applied a fix, as suggested by GCC docs at https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html

Again, IMHO, why not then apply the same fix, e.g. bufsize - strlen (buf) - 1, to the other 4 instances as they are all dealing with real strings and not byte sequences?

Also documenting that further use of strncpy must adhere to this standard fix.

[balazsracz]

the point is that our use of strncpy in the SimpleNodeInfo is correct. These are fixed size char[] fields, that have to be null terminated only if the value is shorter than the fixed size. In fact we also have to fill them with null bytes if they are shorter. So we can only add a trailing zero if we purposefully reduce the maximum transferable string length by one compared to what the protocol defines.

[dpharris]

"So we can only add a trailing zero if we purposefully reduce the maximum transferable string length by one compared to what the protocol defines."

Can you not increase the buffer by one 0 character, and retain the size?

David

On Thu, Sep 3, 2020 at 12:23 PM Balazs Racz notifications@github.com wrote:

the point is that our use of strncpy in the SimpleNodeInfo is correct. These are fixed size char[] fields, that have to be null terminated only if the value is shorter than the fixed size. In fact we also have to fill them with null bytes if they are shorter. So we can only add a trailing zero if we purposefully reduce the maximum transferable string length by one compared to what the protocol defines.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bakerstu/openmrn/issues/424#issuecomment-686708122, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEDQSTVEEWRIUXLCRPULEDSD7UMVANCNFSM4QUB3IHA .

[kiwi64ajs]

Well, to me that feels like we need an OpenMRN specific fixed-length-string-copy-with-null-padding function and handle the protocol specific use cases directly.

Alex

Sent from my iPad

On 4/09/2020, at 7:23 AM, Balazs Racz notifications@github.com wrote:

 the point is that our use of strncpy in the SimpleNodeInfo is correct. These are fixed size char[] fields, that have to be null terminated only if the value is shorter than the fixed size. In fact we also have to fill them with null bytes if they are shorter. So we can only add a trailing zero if we purposefully reduce the maximum transferable string length by one compared to what the protocol defines. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[balazsracz]

I re-read the protocol standard for SNIP. My statement above was wrong, we do need a trailing zero in all cases. Therefore the right fix is indeed to strncpy to one shorter length then add an extra zero. I sent this as a PR.