From: Andre McCurdy <armccurdy@gmail.com>
To: musl@lists.openwall.com
Subject: Re: [PATCH] drop unused extra char from getnameinfo() local buffer
Date: Fri, 28 Jun 2019 12:24:40 -0700 [thread overview]
Message-ID: <CAJ86T=Wo0V8-sb0unHwyNEQfoeTsxkBapRhbr+RVzN1rW5AULw@mail.gmail.com> (raw)
In-Reply-To: <20190628151350.GE1506@brightrain.aerifal.cx>
On Fri, Jun 28, 2019 at 8:14 AM Rich Felker <dalias@libc.org> 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
prev parent reply other threads:[~2019-06-28 19:24 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-28 0:54 Andre McCurdy
2019-06-28 15:13 ` Rich Felker
2019-06-28 19:24 ` Andre McCurdy [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAJ86T=Wo0V8-sb0unHwyNEQfoeTsxkBapRhbr+RVzN1rW5AULw@mail.gmail.com' \
--to=armccurdy@gmail.com \
--cc=musl@lists.openwall.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/musl/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).