[PATCH] drop unused extra char from getnameinfo() local buffer

[PATCH] drop unused extra char from getnameinfo() local buffer

[Andre McCurdy]

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)];
 	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);
-- 
1.9.1

Re: [PATCH] drop unused extra char from getnameinfo() local buffer

[Rich Felker]

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

Re: [PATCH] drop unused extra char from getnameinfo() local buffer

[Andre McCurdy]

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