Date: Mon, 17 Aug 2020 19:54:54 -0400 From: Rich Felker <dalias@...c.org> To: Waldek Kozaczuk <jwkozaczuk@...il.com> Cc: musl@...ts.openwall.com Subject: Re: Both network/if_nametoindex.c and network/if_indextoname.c should use strlcpy instead of strncpy On Mon, Aug 17, 2020 at 12:12:30AM -0400, Waldek Kozaczuk wrote: > Hi, > > As I have been working on upgrading OSv ( > https://github.com/cloudius-systems/osv) to latest version of musl, I have > noticed that both network/if_nametoindex.c and network/if_indextoname.c use > strncpy() to copy interface name to/from buffer. In both cases per > https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/net_if.h.html and > https://linux.die.net/man/3/if_indextoname, it seems that ifname should be > big enough to hold IF_NAMESIZE bytes which SHOULD include null terminating > one. If that is the case both functions should use strlcpy instead of > strncpy. > > Am I wrong? This is a good question. I was under the impression that the kernel (struct ifreq) field was a null-padded string, rather than null-terminated, in which case strncpy would be semantically correct for writing it. However, despite not actually saying it's a string (this looks like an omission in POSIX), the name reported by if_indextoname is supposed to be string, and IF_NAMESIZE is specified to include null termination. I didn't dig back into deep history, and I kinda suspect Linux had this wrong at some point in ancient history (allowing the full IFNAMSIZ bytes to contain significant characters), but nowadays Linux truncates the ifreq field at IFNAMSIZ-1 unconditionally. This means any result we get back from the kernel will be null-terminated and fit in the buffer with strncpy. So as far as I can tell, there is no bug. As to whether it would make sense to make this more explicit, maybe. There doesn't seem to be any harm in doing the null-padding either direction, but it also doesn't seem to be needed (although maybe at some point it was for passing names to kernel?). My leaning would be to keep strncpy for the to-kernel direction (if_nametoindex) and use something that doesn't null-pad for the other direction. Note however that strlcpy can't be used because it's not in the namespace available to these functions. It also has gratuitous logic to compute the length of the uncopied tail that's not needed here. So just sticking with strncpy might still be best... > PS. Trying to compile if_nameindex() yields warning: > > CC musl/src/network/if_nameindex.c > include/api/net/if.h: In function ‘if_nametoindex’: > musl/src/network/if_nametoindex.c:14:2: error: ‘strncpy’ specified bound 16 > equals destination size [-Werror=stringop-truncation] > 14 | strncpy(ifr.ifr_name, name, sizeof ifr.ifr_name); This warning is pretty much always wrong. The only correct places to use strncpy -- places where you're copying into a null-padded buffer that might take up the full buffer without termination -- guarantee that it will be triggered. The warning assumes you don't actually know what strncpy is for that that you're trying to get strlcpy-like semantics. Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.