Re: [musl] Both network/if_nametoindex.c and network/if_indextoname.c should use strlcpy instead of strncpy

[Rich Felker <dalias@libc.org>]

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