intel / intel/openlldp

Verify and correct the size of IFNAMSIZ strings

Open
#108 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
C
Stars
58
Forks
53
PR merge metrics
No merged PRs in 30d

Description

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]

[1]: https://www.gnu.org/software/libc/manual/html_node/Interface-Naming.html#index-IFNAMSIZ

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.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.