From patchwork Tue Oct 10 13:01:58 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 23424 Received: (qmail 68704 invoked by alias); 10 Oct 2017 13:02:08 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 67292 invoked by uid 89); 10 Oct 2017 13:02:05 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=Round, enlarge X-HELO: mx1.redhat.com DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com A3564C04AC74 Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=fweimer@redhat.com Subject: Re: [PATCH] nss_files: Use struct scratch_buffer for gethostbyname [BZ #18023] From: Florian Weimer To: Adhemerval Zanella , libc-alpha@sourceware.org References: <20170904173123.9C550439942E3@oldenburg.str.redhat.com> <426c9fe6-2b88-96c0-2382-651cf30f2db8@linaro.org> <81d77843-06c8-ec51-23b1-542a90d5af4e@redhat.com> Message-ID: <864301dd-4e83-30ec-d690-b2dbaa3864dc@redhat.com> Date: Tue, 10 Oct 2017 15:01:58 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <81d77843-06c8-ec51-23b1-542a90d5af4e@redhat.com> 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 [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 #include #include +#include /* 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; }