* [PATCH] drop unused extra char from getnameinfo() local buffer @ 2019-06-28 0:54 Andre McCurdy 2019-06-28 15:13 ` Rich Felker 0 siblings, 1 reply; 3+ messages in thread From: Andre McCurdy @ 2019-06-28 0:54 UTC (permalink / raw) To: musl; +Cc: 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] drop unused extra char from getnameinfo() local buffer 2019-06-28 0:54 [PATCH] drop unused extra char from getnameinfo() local buffer Andre McCurdy @ 2019-06-28 15:13 ` Rich Felker 2019-06-28 19:24 ` Andre McCurdy 0 siblings, 1 reply; 3+ messages in thread From: Rich Felker @ 2019-06-28 15:13 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] drop unused extra char from getnameinfo() local buffer 2019-06-28 15:13 ` Rich Felker @ 2019-06-28 19:24 ` Andre McCurdy 0 siblings, 0 replies; 3+ messages in thread From: Andre McCurdy @ 2019-06-28 19:24 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-06-28 19:24 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-28 0:54 [PATCH] drop unused extra char from getnameinfo() local buffer Andre McCurdy 2019-06-28 15:13 ` Rich Felker 2019-06-28 19:24 ` Andre McCurdy
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).