| Message ID | 20170904173123.9C550439942E3@oldenburg.str.redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | nss_files: Use struct scratch_buffer for gethostbyname [BZ #18023] | expand |
On 04/09/2017 14:31, Florian Weimer wrote: > 2017-09-04 Florian Weimer <fweimer@redhat.com> > > [BZ #18023] > * nss/nss_files/files-hosts.c (gethostbyname3_multi): Use struct > scratch_buffer. LGTM with some nits below. > > diff --git a/nss/nss_files/files-hosts.c b/nss/nss_files/files-hosts.c > index 867c10c2ef..c2cd07584c 100644 > --- a/nss/nss_files/files-hosts.c > +++ b/nss/nss_files/files-hosts.c > @@ -22,6 +22,7 @@ > #include <arpa/nameser.h> > #include <netdb.h> > #include <resolv/resolv-internal.h> > +#include <scratch_buffer.h> > > > /* Get implementation for some internal functions. */ > @@ -121,15 +122,12 @@ gethostbyname3_multi (FILE * stream, const char *name, int af, > int *errnop, int *herrnop, int flags) > { > /* We have to get all host entries from the file. */ > - size_t tmp_buflen = MIN (buflen, 4096); > - char tmp_buffer_stack[tmp_buflen] > - __attribute__ ((__aligned__ (__alignof__ (struct hostent_data)))); I can't really tell how important is the alignment of this buffer in particular, since on subsequent 'internal_getent' it does uses a plain char buffer. Do we need to keep the alignment of this buffer? > - char *tmp_buffer = tmp_buffer_stack; > + struct scratch_buffer tmp_buffer; > + scratch_buffer_init (&tmp_buffer); > struct hostent tmp_result_buf; > int naddrs = 1; > int naliases = 0; > char *bufferend; > - bool tmp_buffer_malloced = false; > enum nss_status status; > > while (result->h_aliases[naliases] != NULL) > @@ -138,8 +136,8 @@ gethostbyname3_multi (FILE * stream, const char *name, int af, > bufferend = (char *) &result->h_aliases[naliases + 1]; > > again: > - while ((status = internal_getent (stream, &tmp_result_buf, tmp_buffer, > - tmp_buflen, errnop, herrnop, af, > + while ((status = internal_getent (stream, &tmp_result_buf, tmp_buffer.data, > + tmp_buffer.length, errnop, herrnop, af, > flags)) > == NSS_STATUS_SUCCESS) > { > @@ -266,52 +264,18 @@ gethostbyname3_multi (FILE * stream, const char *name, int af, > > if (status == NSS_STATUS_TRYAGAIN) > { > - size_t newsize = 2 * tmp_buflen; > - if (tmp_buffer_malloced) > + if (!scratch_buffer_grow (&tmp_buffer)) > { > - char *newp = realloc (tmp_buffer, newsize); > - if (newp != NULL) > - { > - assert ((((uintptr_t) newp) > - & (__alignof__ (struct hostent_data) - 1)) > - == 0); > - tmp_buffer = newp; > - tmp_buflen = newsize; > - goto again; > - } > - } > - else if (!__libc_use_alloca (buflen + newsize)) > - { > - tmp_buffer = malloc (newsize); > - if (tmp_buffer != NULL) > - { > - assert ((((uintptr_t) tmp_buffer) > - & (__alignof__ (struct hostent_data) - 1)) > - == 0); > - tmp_buffer_malloced = true; > - tmp_buflen = newsize; > - goto again; > - } > + *herrnop = NETDB_INTERNAL; > + status = NSS_STATUS_TRYAGAIN; > } > else > - { > - tmp_buffer > - = extend_alloca (tmp_buffer, tmp_buflen, > - newsize > - + __alignof__ (struct hostent_data)); > - tmp_buffer = (char *) (((uintptr_t) tmp_buffer > - + __alignof__ (struct hostent_data) > - - 1) > - & ~(__alignof__ (struct hostent_data) > - - 1)); > - goto again; > - } > + goto again; > } > else > status = NSS_STATUS_SUCCESS; > out: > - if (tmp_buffer_malloced) > - free (tmp_buffer); > + scratch_buffer_free (&tmp_buffer); > return status; > } I do think this it is easier to read and follow the code *without* the goto, something like: scratch_buffer_init (...); while (1) { while ((status = internal_getent (...)) == NSS_STATUS_SUCCESS) { ... } if (status == NSS_STATUS_TRYAGAIN) if (!scratch_buffer_grow (&tmp_buffer)) { *herrnop = NETDB_INTERNAL; status = NSS_STATUS_TRYAGAIN; break; } else status = NSS_STATUS_SUCCESS; } scratch_buffer_free (...);
On 09/05/2017 07:53 PM, Adhemerval Zanella wrote: >> /* Get implementation for some internal functions. */ >> @@ -121,15 +122,12 @@ gethostbyname3_multi (FILE * stream, const char *name, int af, >> int *errnop, int *herrnop, int flags) >> { >> /* We have to get all host entries from the file. */ >> - size_t tmp_buflen = MIN (buflen, 4096); >> - char tmp_buffer_stack[tmp_buflen] >> - __attribute__ ((__aligned__ (__alignof__ (struct hostent_data)))); > > I can't really tell how important is the alignment of this buffer in particular, > since on subsequent 'internal_getent' it does uses a plain char buffer. Do we > need to keep the alignment of this buffer? internal_getent must align on its own because it is called with user buffers and POSIX doesn't say anything about the alignment. But struct scratch_buffer supplies max_align_t alignment anyway, which is at least the alignment of struct hostent_data (by definition). > I do think this it is easier to read and follow the code *without* the goto, > something like: > > scratch_buffer_init (...); > while (1) > { > while ((status = internal_getent (...)) == NSS_STATUS_SUCCESS) > { > ... > } > if (status == NSS_STATUS_TRYAGAIN) > if (!scratch_buffer_grow (&tmp_buffer)) > { > *herrnop = NETDB_INTERNAL; > status = NSS_STATUS_TRYAGAIN; > break; > } > else > status = NSS_STATUS_SUCCESS; > } > scratch_buffer_free (...); Right, I think I'll make this change in the first (refactoring) patch. Thanks, Florian
On 09/05/2017 08:38 PM, Florian Weimer wrote: >> I do think this it is easier to read and follow the code *without* the goto, >> something like: >> >> scratch_buffer_init (...); >> while (1) >> { >> while ((status = internal_getent (...)) == NSS_STATUS_SUCCESS) >> { >> ... >> } >> if (status == NSS_STATUS_TRYAGAIN) >> if (!scratch_buffer_grow (&tmp_buffer)) >> { >> *herrnop = NETDB_INTERNAL; >> status = NSS_STATUS_TRYAGAIN; >> break; >> } >> else >> status = NSS_STATUS_SUCCESS; >> } >> scratch_buffer_free (...); > > Right, I think I'll make this change in the first (refactoring) patch. I made this change in this patch instead. Still okay? Thanks, Florian 2017-10-10 Florian Weimer <fweimer@redhat.com> [BZ #18023] * nss/nss_files/files-hosts.c (gethostbyname3_multi): Use struct scratch_buffer. Eliminate gotos. diff --git a/nss/nss_files/files-hosts.c b/nss/nss_files/files-hosts.c index 867c10c2ef..763fa39a47 100644 --- a/nss/nss_files/files-hosts.c +++ b/nss/nss_files/files-hosts.c @@ -22,6 +22,7 @@ #include <arpa/nameser.h> #include <netdb.h> #include <resolv/resolv-internal.h> +#include <scratch_buffer.h> /* Get implementation for some internal functions. */ @@ -121,15 +122,12 @@ gethostbyname3_multi (FILE * stream, const char *name, int af, int *errnop, int *herrnop, int flags) { /* We have to get all host entries from the file. */ - size_t tmp_buflen = MIN (buflen, 4096); - char tmp_buffer_stack[tmp_buflen] - __attribute__ ((__aligned__ (__alignof__ (struct hostent_data)))); - char *tmp_buffer = tmp_buffer_stack; + struct scratch_buffer tmp_buffer; + scratch_buffer_init (&tmp_buffer); struct hostent tmp_result_buf; int naddrs = 1; int naliases = 0; char *bufferend; - bool tmp_buffer_malloced = false; enum nss_status status; while (result->h_aliases[naliases] != NULL) @@ -137,181 +135,165 @@ gethostbyname3_multi (FILE * stream, const char *name, int af, bufferend = (char *) &result->h_aliases[naliases + 1]; - again: - while ((status = internal_getent (stream, &tmp_result_buf, tmp_buffer, - tmp_buflen, errnop, herrnop, af, - flags)) - == NSS_STATUS_SUCCESS) + while (true) { - int matches = 1; - struct hostent *old_result = result; - result = &tmp_result_buf; - /* The following piece is a bit clumsy but we want to use the - `LOOKUP_NAME_CASE' value. The optimizer should do its - job. */ - do + status = internal_getent (stream, &tmp_result_buf, tmp_buffer.data, + tmp_buffer.length, errnop, herrnop, af, + flags); + /* Enlarge the buffer if necessary. */ + if (status == NSS_STATUS_TRYAGAIN && *herrnop == NETDB_INTERNAL + && *errnop == ERANGE) { - LOOKUP_NAME_CASE (h_name, h_aliases) - result = old_result; - } - while ((matches = 0)); - - if (matches) - { - /* We could be very clever and try to recycle a few bytes - in the buffer instead of generating new arrays. But - we are not doing this here since it's more work than - it's worth. Simply let the user provide a bit bigger - buffer. */ - char **new_h_addr_list; - char **new_h_aliases; - int newaliases = 0; - size_t newstrlen = 0; - int cnt; - - /* Count the new aliases and the length of the strings. */ - while (tmp_result_buf.h_aliases[newaliases] != NULL) - { - char *cp = tmp_result_buf.h_aliases[newaliases]; - ++newaliases; - newstrlen += strlen (cp) + 1; - } - /* If the real name is different add it also to the - aliases. This means that there is a duplication - in the alias list but this is really the user's - problem. */ - if (strcmp (old_result->h_name, - tmp_result_buf.h_name) != 0) - { - ++newaliases; - newstrlen += strlen (tmp_result_buf.h_name) + 1; - } - - /* Make sure bufferend is aligned. */ - assert ((bufferend - (char *) 0) % sizeof (char *) == 0); - - /* Now we can check whether the buffer is large enough. - 16 is the maximal size of the IP address. */ - if (bufferend + 16 + (naddrs + 2) * sizeof (char *) - + roundup (newstrlen, sizeof (char *)) - + (naliases + newaliases + 1) * sizeof (char *) - >= buffer + buflen) - { - *errnop = ERANGE; - *herrnop = NETDB_INTERNAL; - status = NSS_STATUS_TRYAGAIN; - goto out; - } - - new_h_addr_list = - (char **) (bufferend - + roundup (newstrlen, sizeof (char *)) - + 16); - new_h_aliases = - (char **) ((char *) new_h_addr_list - + (naddrs + 2) * sizeof (char *)); - - /* Copy the old data in the new arrays. */ - for (cnt = 0; cnt < naddrs; ++cnt) - new_h_addr_list[cnt] = old_result->h_addr_list[cnt]; - - for (cnt = 0; cnt < naliases; ++cnt) - new_h_aliases[cnt] = old_result->h_aliases[cnt]; - - /* Store the new strings. */ - cnt = 0; - while (tmp_result_buf.h_aliases[cnt] != NULL) - { - new_h_aliases[naliases++] = bufferend; - bufferend = (__stpcpy (bufferend, - tmp_result_buf.h_aliases[cnt]) - + 1); - ++cnt; - } - - if (cnt < newaliases) + if (!scratch_buffer_grow (&tmp_buffer)) { - new_h_aliases[naliases++] = bufferend; - bufferend = __stpcpy (bufferend, - tmp_result_buf.h_name) + 1; + *errnop = ENOMEM; + /* *herrnop and status already have the right value. */ + break; } - - /* Final NULL pointer. */ - new_h_aliases[naliases] = NULL; - - /* Round up the buffer end address. */ - bufferend += (sizeof (char *) - - ((bufferend - (char *) 0) - % sizeof (char *))) % sizeof (char *); - - /* Now the new address. */ - new_h_addr_list[naddrs++] = - memcpy (bufferend, tmp_result_buf.h_addr, - tmp_result_buf.h_length); - - /* Also here a final NULL pointer. */ - new_h_addr_list[naddrs] = NULL; - - /* Store the new array pointers. */ - old_result->h_aliases = new_h_aliases; - old_result->h_addr_list = new_h_addr_list; - - /* Compute the new buffer end. */ - bufferend = (char *) &new_h_aliases[naliases + 1]; - assert (bufferend <= buffer + buflen); - - result = old_result; + /* Loop around and retry with a larger buffer. */ } - } - - if (status == NSS_STATUS_TRYAGAIN) - { - size_t newsize = 2 * tmp_buflen; - if (tmp_buffer_malloced) + else if (status == NSS_STATUS_SUCCESS) { - char *newp = realloc (tmp_buffer, newsize); - if (newp != NULL) + /* A line was read. Check that it matches the search + criteria. */ + + int matches = 1; + struct hostent *old_result = result; + result = &tmp_result_buf; + /* The following piece is a bit clumsy but we want to use + the `LOOKUP_NAME_CASE' value. The optimizer should do + its job. */ + do { - assert ((((uintptr_t) newp) - & (__alignof__ (struct hostent_data) - 1)) - == 0); - tmp_buffer = newp; - tmp_buflen = newsize; - goto again; + LOOKUP_NAME_CASE (h_name, h_aliases) + result = old_result; } - } - else if (!__libc_use_alloca (buflen + newsize)) - { - tmp_buffer = malloc (newsize); - if (tmp_buffer != NULL) + while ((matches = 0)); + + if (matches) { - assert ((((uintptr_t) tmp_buffer) - & (__alignof__ (struct hostent_data) - 1)) - == 0); - tmp_buffer_malloced = true; - tmp_buflen = newsize; - goto again; - } - } + /* We could be very clever and try to recycle a few bytes + in the buffer instead of generating new arrays. But + we are not doing this here since it's more work than + it's worth. Simply let the user provide a bit bigger + buffer. */ + char **new_h_addr_list; + char **new_h_aliases; + int newaliases = 0; + size_t newstrlen = 0; + int cnt; + + /* Count the new aliases and the length of the strings. */ + while (tmp_result_buf.h_aliases[newaliases] != NULL) + { + char *cp = tmp_result_buf.h_aliases[newaliases]; + ++newaliases; + newstrlen += strlen (cp) + 1; + } + /* If the real name is different add it also to the + aliases. This means that there is a duplication + in the alias list but this is really the user's + problem. */ + if (strcmp (old_result->h_name, + tmp_result_buf.h_name) != 0) + { + ++newaliases; + newstrlen += strlen (tmp_result_buf.h_name) + 1; + } + + /* Make sure bufferend is aligned. */ + assert ((bufferend - (char *) 0) % sizeof (char *) == 0); + + /* Now we can check whether the buffer is large enough. + 16 is the maximal size of the IP address. */ + if (bufferend + 16 + (naddrs + 2) * sizeof (char *) + + roundup (newstrlen, sizeof (char *)) + + (naliases + newaliases + 1) * sizeof (char *) + >= buffer + buflen) + { + *errnop = ERANGE; + *herrnop = NETDB_INTERNAL; + status = NSS_STATUS_TRYAGAIN; + break; + } + + new_h_addr_list = + (char **) (bufferend + + roundup (newstrlen, sizeof (char *)) + + 16); + new_h_aliases = + (char **) ((char *) new_h_addr_list + + (naddrs + 2) * sizeof (char *)); + + /* Copy the old data in the new arrays. */ + for (cnt = 0; cnt < naddrs; ++cnt) + new_h_addr_list[cnt] = old_result->h_addr_list[cnt]; + + for (cnt = 0; cnt < naliases; ++cnt) + new_h_aliases[cnt] = old_result->h_aliases[cnt]; + + /* Store the new strings. */ + cnt = 0; + while (tmp_result_buf.h_aliases[cnt] != NULL) + { + new_h_aliases[naliases++] = bufferend; + bufferend = (__stpcpy (bufferend, + tmp_result_buf.h_aliases[cnt]) + + 1); + ++cnt; + } + + if (cnt < newaliases) + { + new_h_aliases[naliases++] = bufferend; + bufferend = __stpcpy (bufferend, + tmp_result_buf.h_name) + 1; + } + + /* Final NULL pointer. */ + new_h_aliases[naliases] = NULL; + + /* Round up the buffer end address. */ + bufferend += (sizeof (char *) + - ((bufferend - (char *) 0) + % sizeof (char *))) % sizeof (char *); + + /* Now the new address. */ + new_h_addr_list[naddrs++] = + memcpy (bufferend, tmp_result_buf.h_addr, + tmp_result_buf.h_length); + + /* Also here a final NULL pointer. */ + new_h_addr_list[naddrs] = NULL; + + /* Store the new array pointers. */ + old_result->h_aliases = new_h_aliases; + old_result->h_addr_list = new_h_addr_list; + + /* Compute the new buffer end. */ + bufferend = (char *) &new_h_aliases[naliases + 1]; + assert (bufferend <= buffer + buflen); + + result = old_result; + } /* If match was found. */ + + /* If no match is found, loop around and fetch another + line. */ + + } /* status == NSS_STATUS_SUCCESS. */ else - { - tmp_buffer - = extend_alloca (tmp_buffer, tmp_buflen, - newsize - + __alignof__ (struct hostent_data)); - tmp_buffer = (char *) (((uintptr_t) tmp_buffer - + __alignof__ (struct hostent_data) - - 1) - & ~(__alignof__ (struct hostent_data) - - 1)); - goto again; - } - } - else + /* internal_getent returned an error. */ + break; + } /* while (true) */ + + /* Propagate the NSS_STATUS_TRYAGAIN error to the caller. It means + that we may not have loaded the complete result. + NSS_STATUS_NOTFOUND, however, means that we reached the end of + the file successfully. */ + if (status != NSS_STATUS_TRYAGAIN) status = NSS_STATUS_SUCCESS; - out: - if (tmp_buffer_malloced) - free (tmp_buffer); + + scratch_buffer_free (&tmp_buffer); return status; }
On 10/10/2017 03:01 PM, Florian Weimer wrote: > On 09/05/2017 08:38 PM, Florian Weimer wrote: > >>> I do think this it is easier to read and follow the code *without* >>> the goto, >>> something like: >>> >>> scratch_buffer_init (...); >>> while (1) >>> { >>> while ((status = internal_getent (...)) == NSS_STATUS_SUCCESS) >>> { >>> ... >>> } >>> if (status == NSS_STATUS_TRYAGAIN) >>> if (!scratch_buffer_grow (&tmp_buffer)) >>> { >>> *herrnop = NETDB_INTERNAL; >>> status = NSS_STATUS_TRYAGAIN; >>> break; >>> } >>> else >>> status = NSS_STATUS_SUCCESS; >>> } >>> scratch_buffer_free (...); >> >> Right, I think I'll make this change in the first (refactoring) patch. > > I made this change in this patch instead. Still okay? I'm going to push this because you've already acked the subsequent patch. Thanks, Florian
On 11/10/2017 02:04, Florian Weimer wrote: > On 10/10/2017 03:01 PM, Florian Weimer wrote: >> On 09/05/2017 08:38 PM, Florian Weimer wrote: >> >>>> I do think this it is easier to read and follow the code *without* the goto, >>>> something like: >>>> >>>> scratch_buffer_init (...); >>>> while (1) >>>> { >>>> while ((status = internal_getent (...)) == NSS_STATUS_SUCCESS) >>>> { >>>> ... >>>> } >>>> if (status == NSS_STATUS_TRYAGAIN) >>>> if (!scratch_buffer_grow (&tmp_buffer)) >>>> { >>>> *herrnop = NETDB_INTERNAL; >>>> status = NSS_STATUS_TRYAGAIN; >>>> break; >>>> } >>>> else >>>> status = NSS_STATUS_SUCCESS; >>>> } >>>> scratch_buffer_free (...); >>> >>> Right, I think I'll make this change in the first (refactoring) patch. >> >> I made this change in this patch instead. Still okay? > > I'm going to push this because you've already acked the subsequent patch. LGTM, I though I had acked this patch as well (I did spend some time yesterday checking the new version).
diff --git a/nss/nss_files/files-hosts.c b/nss/nss_files/files-hosts.c index 867c10c2ef..c2cd07584c 100644 --- a/nss/nss_files/files-hosts.c +++ b/nss/nss_files/files-hosts.c @@ -22,6 +22,7 @@ #include <arpa/nameser.h> #include <netdb.h> #include <resolv/resolv-internal.h> +#include <scratch_buffer.h> /* Get implementation for some internal functions. */ @@ -121,15 +122,12 @@ gethostbyname3_multi (FILE * stream, const char *name, int af, int *errnop, int *herrnop, int flags) { /* We have to get all host entries from the file. */ - size_t tmp_buflen = MIN (buflen, 4096); - char tmp_buffer_stack[tmp_buflen] - __attribute__ ((__aligned__ (__alignof__ (struct hostent_data)))); - char *tmp_buffer = tmp_buffer_stack; + struct scratch_buffer tmp_buffer; + scratch_buffer_init (&tmp_buffer); struct hostent tmp_result_buf; int naddrs = 1; int naliases = 0; char *bufferend; - bool tmp_buffer_malloced = false; enum nss_status status; while (result->h_aliases[naliases] != NULL) @@ -138,8 +136,8 @@ gethostbyname3_multi (FILE * stream, const char *name, int af, bufferend = (char *) &result->h_aliases[naliases + 1]; again: - while ((status = internal_getent (stream, &tmp_result_buf, tmp_buffer, - tmp_buflen, errnop, herrnop, af, + while ((status = internal_getent (stream, &tmp_result_buf, tmp_buffer.data, + tmp_buffer.length, errnop, herrnop, af, flags)) == NSS_STATUS_SUCCESS) { @@ -266,52 +264,18 @@ gethostbyname3_multi (FILE * stream, const char *name, int af, if (status == NSS_STATUS_TRYAGAIN) { - size_t newsize = 2 * tmp_buflen; - if (tmp_buffer_malloced) + if (!scratch_buffer_grow (&tmp_buffer)) { - char *newp = realloc (tmp_buffer, newsize); - if (newp != NULL) - { - assert ((((uintptr_t) newp) - & (__alignof__ (struct hostent_data) - 1)) - == 0); - tmp_buffer = newp; - tmp_buflen = newsize; - goto again; - } - } - else if (!__libc_use_alloca (buflen + newsize)) - { - tmp_buffer = malloc (newsize); - if (tmp_buffer != NULL) - { - assert ((((uintptr_t) tmp_buffer) - & (__alignof__ (struct hostent_data) - 1)) - == 0); - tmp_buffer_malloced = true; - tmp_buflen = newsize; - goto again; - } + *herrnop = NETDB_INTERNAL; + status = NSS_STATUS_TRYAGAIN; } else - { - tmp_buffer - = extend_alloca (tmp_buffer, tmp_buflen, - newsize - + __alignof__ (struct hostent_data)); - tmp_buffer = (char *) (((uintptr_t) tmp_buffer - + __alignof__ (struct hostent_data) - - 1) - & ~(__alignof__ (struct hostent_data) - - 1)); - goto again; - } + goto again; } else status = NSS_STATUS_SUCCESS; out: - if (tmp_buffer_malloced) - free (tmp_buffer); + scratch_buffer_free (&tmp_buffer); return status; }