Verify and correct the size of IFNAMSIZ strings

[penguin359]

While doing some builds with ASAN enabled along with -Werror, I hit a build warning about a strncpy() that the source string might get truncated due to the destination string array being one character shorter. The source array was size IFNAMSIZ+1 and the destination is size IFNAMSIZ which I believe the latter to be the correct size. Interestingly, this error is only caught under GCC 9.x in combination with -D_FORTIFY_SOURCE=2 and newer GCC versions seem to be ignoring it. I believe the correct size should be IFNAMSIZ everywhere and the code should be carefully reviewed to make sure there are no unexpected mismatches. There are a few locations where index IFNAMSIZ is being used to add the nul-terminator, but this should be IFNAMSIZ-1, however, those cases are where they include the extra byte so there are no overruns.

For reference, here is the definition of IFNAMSIZ from the glibc manual:

"This constant defines the maximum buffer size needed to hold an interface name, including its terminating zero byte."1

Here is the text of the error I am getting:

In file included from /usr/include/string.h:495,
                 from lldp/l2_packet_linux.c:29:
In function ‘strncpy’,
    inlined from ‘l2_packet_init’ at lldp/l2_packet_linux.c:186:2:
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: error: ‘__builtin_strncpy’ output may be truncated copying 15 bytes from a string of length 16 [-Werror=stringop-truncation]
  106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I will take a look more carefully and audit the code for any violations. Once I am reasonably confident, I'll submit a PR, but I just wanted to document this issue here.

Actually, I am now wondering if the strncpy() warnings I was trying to silent in PR #107 were due to this same issue. I need to review that and if fixing the mismatch in interface name strings avoids that warning, then I should remove that patch as it's a legit warning.

[penguin359]

I did do a quick look over the latest kernel sources just to compare and they do use IFNAMSIZ without an addition pretty much everywhere including for the size argument of strscpy() in the kernel and it is defined as 16 bytes. The few exceptions to it are where they reserve a few extra bytes for some kind of metadata, but I don't think anything in openlldp uses that extra byte for such purposes.

[liuhangbin]

I also noticed this issue when build with GCC 11.

In file included from /usr/include/string.h:519,
                 from qbg/vdp22_cmds.c:32:
In function 'strncpy',
    inlined from 'get_arg_vsi' at qbg/vdp22_cmds.c:580:2:
/usr/include/bits/string_fortified.h:95:10: warning: 'strncpy' output may be truncated copying 16 bytes from a string of length 16 [-Wstringop-truncation]
   95 |   return __builtin___strncpy_chk (__dest, __src, __len,
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   96 |                                   __glibc_objsize (__dest));
      |                                   ~~~~~~~~~~~~~~~~~~~~~~~~~

I don't understand why I got this warning for STRNCPY_TERMINATED(vsi.ifname, cmd->ifname, sizeof(vsi.ifname)); even we already did strncpy (DEST, SRC, N - 1);.

On the other hand, I think we shouldn't use DEST[N - 1] = '\0'; in STRNCPY_TERMINATED. When N > sizeof(DEST) this will cause buffer overflow. How about using DEST[sizeof(DEST)-1] = '\0'; ?

Or maybe we should implement a version of strlcpy ?