From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=0.3 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SUBJ_OBFU_PUNCT_FEW, SUBJ_OBFU_PUNCT_MANY autolearn=no autolearn_force=no version=3.4.4 Received: (qmail 16852 invoked from network); 17 Aug 2020 23:55:10 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 17 Aug 2020 23:55:10 -0000 Received: (qmail 14019 invoked by uid 550); 17 Aug 2020 23:55:07 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 14001 invoked from network); 17 Aug 2020 23:55:06 -0000 Date: Mon, 17 Aug 2020 19:54:54 -0400 From: Rich Felker To: Waldek Kozaczuk Cc: musl@lists.openwall.com Message-ID: <20200817235454.GC3265@brightrain.aerifal.cx> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] 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