[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 28 Jun 2019 11:13:50 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] drop unused extra char from getnameinfo() local
buffer
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".
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?
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
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.
