From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/14281 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Andre McCurdy Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] drop unused extra char from getnameinfo() local buffer Date: Fri, 28 Jun 2019 12:24:40 -0700 Message-ID: References: <1561683273-18265-1-git-send-email-armccurdy@gmail.com> <20190628151350.GE1506@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="178510"; mail-complaints-to="usenet@blaine.gmane.org" To: musl@lists.openwall.com Original-X-From: musl-return-14297-gllmg-musl=m.gmane.org@lists.openwall.com Fri Jun 28 21:25:07 2019 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.89) (envelope-from ) id 1hgwUd-000kJ9-Ah for gllmg-musl@m.gmane.org; Fri, 28 Jun 2019 21:25:07 +0200 Original-Received: (qmail 11722 invoked by uid 550); 28 Jun 2019 19:25:04 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 11701 invoked from network); 28 Jun 2019 19:25:04 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to; bh=Xq9G7LTXaBSv1vq7oqKBmUFCKalpUvCjqCy8yrrp8X0=; b=ShdQ6NpzqWwGqqbgKWKVjtzHz+TW+/yTo9Zx6ohZTa85oDBgtIgdryX2ez6yLB/VCb sXVtmLN2qvsrMCjP7fytdPHZ/ne21ekZIZ2FKml4EoefLZRjkJ73JEGkkhxwe7QcI36r ZSEppqrg4LbTbnsUWTXeLgLDgXKdKNQV3Qzvecfx1wXC12CIqyJnlyyYr2kDlnf0nRKE FyoUudNF1IcNT1tEqy1H0jJooPrvFvzlJLda+xRQoMtdVQnIZQ7lcqTPD11h/p8gW2SX 68IbKIhiwp7T7MWqXoT7X7up0LBAjK7OQo+0rSH8oQYCHkCFmI1+N1ifE96Y90bysrG5 W5Nw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to; bh=Xq9G7LTXaBSv1vq7oqKBmUFCKalpUvCjqCy8yrrp8X0=; b=MY1p1w2s7/lYmxvmVkgHKm0wv5bGS0To4S+VYWpm+vglzCrxLJ1xHPSn/2hOHH66Fj wD30cnUinXusYnUBl5kiDkS2ljesFoCUwrBozWvj1PzWsImZ9ZJ4Qeht4XX8iB/7FQAf nWjbznsKFdCOgdOzWmJ5D9NR7CSH8UgbwcghQUvdvNFSYbPggUkCZQ5cAp0M+2qEZmnc tm+lfXemzWv5KE4BDLfw0C6MfY0gc4Pd1rKDTRFo1MFZrz2WzsaWznwbaL2OOAG+l6FS ULFvdnBbWOJJ4ixs+fWQhUuSmwbVuFVumsiWXwU1hV4MMRWZB7WhlS4Wn7YWhjcWnwzP ncuA== X-Gm-Message-State: APjAAAUtjuCSRjyYev18j+vMATUORY6PpjwHP4ThPhKFx5KLa3jsXou8 YYJ6cV4MKRWET8zehdSKtA0SD/qj9YiWD13qTQbHiqWS X-Google-Smtp-Source: APXvYqyH8ChbCFcFc7jZtexWApR6kCGMVEKgBPBF43WlIhFgZMRwO+51N1MJ7Yl8HzNh/Yud4s+yy0sEiOEhiTf6qw4= X-Received: by 2002:a1f:3244:: with SMTP id y65mr4408540vky.77.1561749892181; Fri, 28 Jun 2019 12:24:52 -0700 (PDT) In-Reply-To: <20190628151350.GE1506@brightrain.aerifal.cx> Xref: news.gmane.org gmane.linux.lib.musl.general:14281 Archived-At: On Fri, Jun 28, 2019 at 8:14 AM Rich Felker wrote: > On Thu, Jun 27, 2019 at 05:54:33PM -0700, Andre McCurdy wrote: > > The num local buffer is only passed to itoa(), which expects a buffer > > size of 3*sizeof(int), not 3*sizeof(int)+1. Also change the data type > > of the port local variable to clarify that itoa() only handles > > unsigned values. > > --- > > src/network/getnameinfo.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/src/network/getnameinfo.c b/src/network/getnameinfo.c > > index f77e73a..02c2c09 100644 > > --- a/src/network/getnameinfo.c > > +++ b/src/network/getnameinfo.c > > @@ -124,7 +124,7 @@ int getnameinfo(const struct sockaddr *restrict sa, socklen_t sl, > > int flags) > > { > > char ptr[PTR_MAX]; > > - char buf[256], num[3*sizeof(int)+1]; > > + char buf[256], num[3*sizeof(int)]; > > I think the 3*sizeof(int)+1 idiom is a standard one we use throughout > the code, because it's clearly valid for any size int. It's based > ceil(log10(256))==3 and one byte for termination, and it would > actually be sharp in the pathological case sizeof(int)==1 (which of > course we don't support and can't actually be supported on a hosted > implementation due to stdio constaints). > > In practice a constant 11 would work for known-32-bit int, but the > desire here is to be obviously-safe, not to be "optimal". There's an additional subtlety that it needs to be possible to prepend an extra char to the string returned by itoa() - see line 177. A 12 byte buffer allows for that but an 11 byte buffer would not. > I think what you have found though is that the expectation in the > definition of itoa is inconsistent. I probably didn't notice the > inconsistency because of the *--p instead of *p. It should be either > *p=0 or p+=3*sizeof(int)+1 initially, I think. Does that sound right? Yes, either of those would be OK. A possible third solution to resolve the inconsistency would be: --- a/src/network/getnameinfo.c +++ b/src/network/getnameinfo.c @@ -173,7 +173,7 @@ int getnameinfo(const struct sockaddr *restrict sa, socklen_t sl, IN6_IS_ADDR_MC_LINKLOCAL(a))) p = if_indextoname(scopeid, tmp+1); if (!p) - p = itoa(num, scopeid); + p = itoa(num+1, scopeid); *--p = '%'; strcat(buf, p); } > Either way it's harmless on the only value of sizeof(int) that > actually occurs, but I'd like to fix the inconsistency here. > > > int af = sa->sa_family; > > unsigned char *a; > > unsigned scopeid; > > @@ -184,7 +184,7 @@ int getnameinfo(const struct sockaddr *restrict sa, socklen_t sl, > > > > if (serv && servlen) { > > char *p = buf; > > - int port = ntohs(((struct sockaddr_in *)sa)->sin_port); > > + unsigned port = ntohs(((struct sockaddr_in *)sa)->sin_port); > > buf[0] = 0; > > if (!(flags & NI_NUMERICSERV)) > > reverse_services(buf, port, flags & NI_DGRAM); > > This is ok-ish since it's consistent with the signature for itoa, but > the range of value is such that it can never be negative either way. > > Rich