Message ID | 20180626154633.BC8B943994575@oldenburg.str.redhat.com |
---|---|
State | New |
Headers | show |
Series | gethostid (Linux variant): Switch to struct scratch_buffer [BZ #18023] | expand |
On 26/06/2018 12:46, Florian Weimer wrote: > Previously, extend_alloca was used without alloca accounting, > which could have been problematic with large NSS results. > > 2018-06-26 Florian Weimer <fweimer@redhat.com> > > [BZ #18023] > * sysdeps/unix/sysv/linux/gethostid.c (gethostid): Use struct > scratch_buffer instead of extend_alloca. Update comments. > > diff --git a/sysdeps/unix/sysv/linux/gethostid.c b/sysdeps/unix/sysv/linux/gethostid.c > index 73c56e57e5..c8a33fe14d 100644 > --- a/sysdeps/unix/sysv/linux/gethostid.c > +++ b/sysdeps/unix/sysv/linux/gethostid.c > @@ -21,6 +21,7 @@ > #include <unistd.h> > #include <netdb.h> > #include <not-cancel.h> > +#include <stdbool.h> > > #define HOSTIDFILE "/etc/hostid" > > @@ -63,13 +64,12 @@ sethostid (long int id) > # include <sys/param.h> > # include <resolv/netdb.h> > # include <netinet/in.h> > +# include <scratch_buffer.h> > > long int > gethostid (void) > { > char hostname[MAXHOSTNAMELEN + 1]; > - size_t buflen; > - char *buffer; > struct hostent hostbuf, *hp; > int32_t id; > struct in_addr in; > @@ -88,29 +88,40 @@ gethostid (void) > return id; > } > > - /* Getting from the file was not successful. An intelligent guess for > - a unique number of a host is its IP address. Return this. */ > + /* Getting from the file was not successful. An intelligent guess > + for a unique number of a host is its IP address. To get the IP > + address we need to know the host name. */ > if (__gethostname (hostname, MAXHOSTNAMELEN) < 0 || hostname[0] == '\0') > /* This also fails. Return and arbitrary value. */ > return 0; > > - buflen = 1024; > - buffer = __alloca (buflen); > - > - /* To get the IP address we need to know the host name. */ > - while (__gethostbyname_r (hostname, &hostbuf, buffer, buflen, &hp, &herr) > - != 0 > - || hp == NULL) > - if (herr != NETDB_INTERNAL || errno != ERANGE) > - return 0; > - else > - /* Enlarge buffer. */ > - buffer = extend_alloca (buffer, buflen, 2 * buflen); > + /* Determine the IP address of the host name. */ > + struct scratch_buffer tmpbuf; > + scratch_buffer_init (&tmpbuf); > + while (true) > + { > + int ret = __gethostbyname_r (hostname, &hostbuf, > + tmpbuf.data, tmpbuf.length, &hp, &herr); > + if (ret == 0) > + break; > + else > + { > + /* Enlarge the buffer on ERANGE. */ > + if (herr == NETDB_INTERNAL && errno == ERANGE) > + { > + if (!scratch_buffer_grow (&tmpbuf)) > + return 0; > + } > + else > + /* Other errors are a failure. Return an arbitrary value. */ Shouldn' it call 'scratch_buffer_free' here for the case the buffer is grown and a subsequent __gethostbyname_r results something different than ERANGE (assuming it is possible)? > + return 0; > + } > + } > > in.s_addr = 0; > memcpy (&in, hp->h_addr, > (int) sizeof (in) < hp->h_length ? (int) sizeof (in) : hp->h_length); > - > + scratch_buffer_free (&tmpbuf); > /* For the return value to be not exactly the IP address we do some > bit fiddling. */ > return (int32_t) (in.s_addr << 16 | in.s_addr >> 16); >
On 06/26/2018 06:58 PM, Adhemerval Zanella wrote: >> + /* Determine the IP address of the host name. */ >> + struct scratch_buffer tmpbuf; >> + scratch_buffer_init (&tmpbuf); >> + while (true) >> + { >> + int ret = __gethostbyname_r (hostname, &hostbuf, >> + tmpbuf.data, tmpbuf.length, &hp, &herr); >> + if (ret == 0) >> + break; >> + else >> + { >> + /* Enlarge the buffer on ERANGE. */ >> + if (herr == NETDB_INTERNAL && errno == ERANGE) >> + { >> + if (!scratch_buffer_grow (&tmpbuf)) >> + return 0; >> + } >> + else >> + /* Other errors are a failure. Return an arbitrary value. */ > Shouldn' it call 'scratch_buffer_free' here for the case the buffer is > grown and a subsequent __gethostbyname_r results something different > than ERANGE (assuming it is possible)? Thanks, you are right. New patch attached. Florian Subject: [PATCH] gethostid (Linux variant): Switch to struct scratch_buffer [BZ #18023] To: libc-alpha@sourceware.org Previously, extend_alloca was used without alloca accounting, which could have been problematic with large NSS results. 2018-06-26 Florian Weimer <fweimer@redhat.com> [BZ #18023] * sysdeps/unix/sysv/linux/gethostid.c (gethostid): Use struct scratch_buffer instead of extend_alloca. Update comments. diff --git a/sysdeps/unix/sysv/linux/gethostid.c b/sysdeps/unix/sysv/linux/gethostid.c index 73c56e57e5..2e20f034dc 100644 --- a/sysdeps/unix/sysv/linux/gethostid.c +++ b/sysdeps/unix/sysv/linux/gethostid.c @@ -21,6 +21,7 @@ #include <unistd.h> #include <netdb.h> #include <not-cancel.h> +#include <stdbool.h> #define HOSTIDFILE "/etc/hostid" @@ -63,13 +64,12 @@ sethostid (long int id) # include <sys/param.h> # include <resolv/netdb.h> # include <netinet/in.h> +# include <scratch_buffer.h> long int gethostid (void) { char hostname[MAXHOSTNAMELEN + 1]; - size_t buflen; - char *buffer; struct hostent hostbuf, *hp; int32_t id; struct in_addr in; @@ -88,29 +88,43 @@ gethostid (void) return id; } - /* Getting from the file was not successful. An intelligent guess for - a unique number of a host is its IP address. Return this. */ + /* Getting from the file was not successful. An intelligent guess + for a unique number of a host is its IP address. To get the IP + address we need to know the host name. */ if (__gethostname (hostname, MAXHOSTNAMELEN) < 0 || hostname[0] == '\0') /* This also fails. Return and arbitrary value. */ return 0; - buflen = 1024; - buffer = __alloca (buflen); - - /* To get the IP address we need to know the host name. */ - while (__gethostbyname_r (hostname, &hostbuf, buffer, buflen, &hp, &herr) - != 0 - || hp == NULL) - if (herr != NETDB_INTERNAL || errno != ERANGE) - return 0; - else - /* Enlarge buffer. */ - buffer = extend_alloca (buffer, buflen, 2 * buflen); + /* Determine the IP address of the host name. */ + struct scratch_buffer tmpbuf; + scratch_buffer_init (&tmpbuf); + while (true) + { + int ret = __gethostbyname_r (hostname, &hostbuf, + tmpbuf.data, tmpbuf.length, &hp, &herr); + if (ret == 0) + break; + else + { + /* Enlarge the buffer on ERANGE. */ + if (herr == NETDB_INTERNAL && errno == ERANGE) + { + if (!scratch_buffer_grow (&tmpbuf)) + return 0; + } + /* Other errors are a failure. Return an arbitrary value. */ + else + { + scratch_buffer_free (&tmpbuf); + return 0; + } + } + } in.s_addr = 0; memcpy (&in, hp->h_addr, (int) sizeof (in) < hp->h_length ? (int) sizeof (in) : hp->h_length); - + scratch_buffer_free (&tmpbuf); /* For the return value to be not exactly the IP address we do some bit fiddling. */ return (int32_t) (in.s_addr << 16 | in.s_addr >> 16);
On 26/06/2018 14:33, Florian Weimer wrote: > On 06/26/2018 06:58 PM, Adhemerval Zanella wrote: >>> + /* Determine the IP address of the host name. */ >>> + struct scratch_buffer tmpbuf; >>> + scratch_buffer_init (&tmpbuf); >>> + while (true) >>> + { >>> + int ret = __gethostbyname_r (hostname, &hostbuf, >>> + tmpbuf.data, tmpbuf.length, &hp, &herr); >>> + if (ret == 0) >>> + break; >>> + else >>> + { >>> + /* Enlarge the buffer on ERANGE. */ >>> + if (herr == NETDB_INTERNAL && errno == ERANGE) >>> + { >>> + if (!scratch_buffer_grow (&tmpbuf)) >>> + return 0; >>> + } >>> + else >>> + /* Other errors are a failure. Return an arbitrary value. */ >> Shouldn' it call 'scratch_buffer_free' here for the case the buffer is >> grown and a subsequent __gethostbyname_r results something different >> than ERANGE (assuming it is possible)? > > Thanks, you are right. New patch attached. > > Florian New version LGTM, thanks.
On Jun 26 2018, Florian Weimer <fweimer@redhat.com> wrote: > @@ -88,29 +88,43 @@ gethostid (void) > return id; > } > > - /* Getting from the file was not successful. An intelligent guess for > - a unique number of a host is its IP address. Return this. */ > + /* Getting from the file was not successful. An intelligent guess > + for a unique number of a host is its IP address. To get the IP > + address we need to know the host name. */ > if (__gethostname (hostname, MAXHOSTNAMELEN) < 0 || hostname[0] == '\0') > /* This also fails. Return and arbitrary value. */ > return 0; > > - buflen = 1024; > - buffer = __alloca (buflen); > - > - /* To get the IP address we need to know the host name. */ > - while (__gethostbyname_r (hostname, &hostbuf, buffer, buflen, &hp, &herr) > - != 0 > - || hp == NULL) > - if (herr != NETDB_INTERNAL || errno != ERANGE) > - return 0; > - else > - /* Enlarge buffer. */ > - buffer = extend_alloca (buffer, buflen, 2 * buflen); > + /* Determine the IP address of the host name. */ > + struct scratch_buffer tmpbuf; > + scratch_buffer_init (&tmpbuf); > + while (true) > + { > + int ret = __gethostbyname_r (hostname, &hostbuf, > + tmpbuf.data, tmpbuf.length, &hp, &herr); > + if (ret == 0) > + break; That fails to handle hp == NULL any more, see bug 23679. Andreas.
* Andreas Schwab: > On Jun 26 2018, Florian Weimer <fweimer@redhat.com> wrote: > >> @@ -88,29 +88,43 @@ gethostid (void) >> return id; >> } >> >> - /* Getting from the file was not successful. An intelligent guess for >> - a unique number of a host is its IP address. Return this. */ >> + /* Getting from the file was not successful. An intelligent guess >> + for a unique number of a host is its IP address. To get the IP >> + address we need to know the host name. */ >> if (__gethostname (hostname, MAXHOSTNAMELEN) < 0 || hostname[0] == '\0') >> /* This also fails. Return and arbitrary value. */ >> return 0; >> >> - buflen = 1024; >> - buffer = __alloca (buflen); >> - >> - /* To get the IP address we need to know the host name. */ >> - while (__gethostbyname_r (hostname, &hostbuf, buffer, buflen, &hp, &herr) >> - != 0 >> - || hp == NULL) >> - if (herr != NETDB_INTERNAL || errno != ERANGE) >> - return 0; >> - else >> - /* Enlarge buffer. */ >> - buffer = extend_alloca (buffer, buflen, 2 * buflen); >> + /* Determine the IP address of the host name. */ >> + struct scratch_buffer tmpbuf; >> + scratch_buffer_init (&tmpbuf); >> + while (true) >> + { >> + int ret = __gethostbyname_r (hostname, &hostbuf, >> + tmpbuf.data, tmpbuf.length, &hp, &herr); >> + if (ret == 0) >> + break; > > That fails to handle hp == NULL any more, see bug 23679. Oops. Do you want me to write a patch?
On Sep 18 2018, Florian Weimer <fw@deneb.enyo.de> wrote:
> Oops. Do you want me to write a patch?
There is one attached to the bug.
Andreas.
* Andreas Schwab: > On Sep 18 2018, Florian Weimer <fw@deneb.enyo.de> wrote: > >> Oops. Do you want me to write a patch? > > There is one attached to the bug. Right, I'll take care of it.
diff --git a/sysdeps/unix/sysv/linux/gethostid.c b/sysdeps/unix/sysv/linux/gethostid.c index 73c56e57e5..c8a33fe14d 100644 --- a/sysdeps/unix/sysv/linux/gethostid.c +++ b/sysdeps/unix/sysv/linux/gethostid.c @@ -21,6 +21,7 @@ #include <unistd.h> #include <netdb.h> #include <not-cancel.h> +#include <stdbool.h> #define HOSTIDFILE "/etc/hostid" @@ -63,13 +64,12 @@ sethostid (long int id) # include <sys/param.h> # include <resolv/netdb.h> # include <netinet/in.h> +# include <scratch_buffer.h> long int gethostid (void) { char hostname[MAXHOSTNAMELEN + 1]; - size_t buflen; - char *buffer; struct hostent hostbuf, *hp; int32_t id; struct in_addr in; @@ -88,29 +88,40 @@ gethostid (void) return id; } - /* Getting from the file was not successful. An intelligent guess for - a unique number of a host is its IP address. Return this. */ + /* Getting from the file was not successful. An intelligent guess + for a unique number of a host is its IP address. To get the IP + address we need to know the host name. */ if (__gethostname (hostname, MAXHOSTNAMELEN) < 0 || hostname[0] == '\0') /* This also fails. Return and arbitrary value. */ return 0; - buflen = 1024; - buffer = __alloca (buflen); - - /* To get the IP address we need to know the host name. */ - while (__gethostbyname_r (hostname, &hostbuf, buffer, buflen, &hp, &herr) - != 0 - || hp == NULL) - if (herr != NETDB_INTERNAL || errno != ERANGE) - return 0; - else - /* Enlarge buffer. */ - buffer = extend_alloca (buffer, buflen, 2 * buflen); + /* Determine the IP address of the host name. */ + struct scratch_buffer tmpbuf; + scratch_buffer_init (&tmpbuf); + while (true) + { + int ret = __gethostbyname_r (hostname, &hostbuf, + tmpbuf.data, tmpbuf.length, &hp, &herr); + if (ret == 0) + break; + else + { + /* Enlarge the buffer on ERANGE. */ + if (herr == NETDB_INTERNAL && errno == ERANGE) + { + if (!scratch_buffer_grow (&tmpbuf)) + return 0; + } + else + /* Other errors are a failure. Return an arbitrary value. */ + return 0; + } + } in.s_addr = 0; memcpy (&in, hp->h_addr, (int) sizeof (in) < hp->h_length ? (int) sizeof (in) : hp->h_length); - + scratch_buffer_free (&tmpbuf); /* For the return value to be not exactly the IP address we do some bit fiddling. */ return (int32_t) (in.s_addr << 16 | in.s_addr >> 16);