This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
[PATCH 1.2] Clean up netgroupcache
- From: OndÅej BÃlka <neleai at seznam dot cz>
- To: Siddhesh Poyarekar <siddhesh at redhat dot com>
- Cc: Mike Frysinger <vapier at gentoo dot org>, libc-alpha at sourceware dot org
- Date: Fri, 11 Apr 2014 16:30:00 +0200
- Subject: [PATCH 1.2] Clean up netgroupcache
- Authentication-results: sourceware.org; auth=none
- References: <20140327040406 dot GA26264 at spoyarek dot pnq dot redhat dot com> <1499542 dot yzGAIksTkn at vapier> <20140327192254 dot GC1982 at domone dot podge> <20140327203207 dot GA2476 at domone dot podge> <20140327211516 dot GB2476 at domone dot podge> <20140328024737 dot GJ31211 at spoyarek dot pnq dot redhat dot com>
On Fri, Mar 28, 2014 at 08:17:37AM +0530, Siddhesh Poyarekar wrote:
> On Thu, Mar 27, 2014 at 10:15:16PM +0100, OndÅej BÃlka wrote:
> > Had typo there, here is fixed patch.
>
> Please add a note on how you tested the fix.
>
> > * nscd/netgroupcache.c: Clean up implementation.
>
> Incorrect ChangeLog entry.
>
New one below.
> > - || nhost > nuser || nuser > ndomain)
> > + size_t newlen = buffilled + hostlen + userlen + domainlen;
> > + if (newlen + req->key_len > buflen)
>
> Might as well put req->key_len in newlen and simplify this further.
>
ok
> > @@ -328,8 +289,8 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
> > }
> > else if (status == NSS_STATUS_UNAVAIL && e == ERANGE)
> > {
> > - buflen *= 2;
> > - buffer = xrealloc (buffer, buflen);
> > + tempbuflen *= 2;
> > + tempbuf = xrealloc (tempbuf, tempbuflen);
>
> This ought to be unlikely now, since the consolidated lengths of user,
> host and domain should fit in the initially allocated 1024 bytes.
>
I made change that previous branch is likely. I am not sure about this
one because its conditional probability. This condition is checked only
on failure. So likeliness is determined by probability that failure was
caused by insufficient size.
New version is here.
* nscd/netgroupcache.c (addgetnetgrentX): Use separate buffer
for result and temporary data.
diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c
index 820d823..9582ffb 100644
--- a/nscd/netgroupcache.c
+++ b/nscd/netgroupcache.c
@@ -138,8 +138,10 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
char *key_copy = NULL;
struct __netgrent data;
size_t buflen = MAX (1024, sizeof (*dataset) + req->key_len);
+ size_t tempbuflen = 1024;
size_t buffilled = sizeof (*dataset);
char *buffer = NULL;
+ char *tempbuf;
size_t nentries = 0;
size_t group_len = strlen (key) + 1;
union
@@ -159,6 +161,8 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
memset (&data, '\0', sizeof (data));
buffer = xmalloc (buflen);
+ tempbuf = xmalloc (tempbuflen);
+
first_needed.elem.next = &first_needed.elem;
memcpy (first_needed.elem.name, key, group_len);
data.needed_groups = &first_needed.elem;
@@ -201,14 +205,13 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
while (1)
{
int e;
- status = getfct.f (&data, buffer + buffilled,
- buflen - buffilled - req->key_len, &e);
+ status = getfct.f (&data, tempbuf, tempbuflen, &e);
if (status == NSS_STATUS_RETURN
|| status == NSS_STATUS_NOTFOUND)
/* This was either the last one for this group or the
group was empty. Look at next group if available. */
break;
- if (status == NSS_STATUS_SUCCESS)
+ if (__glibc_likely (status == NSS_STATUS_SUCCESS))
{
if (data.type == triple_val)
{
@@ -220,53 +223,13 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
size_t userlen = strlen (nuser ?: "") + 1;
size_t domainlen = strlen (ndomain ?: "") + 1;
- if (nhost == NULL || nuser == NULL || ndomain == NULL
- || nhost > nuser || nuser > ndomain)
- {
- const char *last = nhost;
- if (last == NULL
- || (nuser != NULL && nuser > last))
- last = nuser;
- if (last == NULL
- || (ndomain != NULL && ndomain > last))
- last = ndomain;
-
- size_t bufused
- = (last == NULL
- ? buffilled
- : last + strlen (last) + 1 - buffer);
-
- /* We have to make temporary copies. */
- size_t needed = hostlen + userlen + domainlen;
-
- if (buflen - req->key_len - bufused < needed)
- {
- buflen += MAX (buflen, 2 * needed);
- /* Save offset in the old buffer. We don't
- bother with the NULL check here since
- we'll do that later anyway. */
- size_t nhostdiff = nhost - buffer;
- size_t nuserdiff = nuser - buffer;
- size_t ndomaindiff = ndomain - buffer;
-
- char *newbuf = xrealloc (buffer, buflen);
- /* Fix up the triplet pointers into the new
- buffer. */
- nhost = (nhost ? newbuf + nhostdiff
- : NULL);
- nuser = (nuser ? newbuf + nuserdiff
- : NULL);
- ndomain = (ndomain ? newbuf + ndomaindiff
- : NULL);
- buffer = newbuf;
- }
+ size_t newlen = buffilled + hostlen + userlen
+ + domainlen + req->key_len;
- nhost = memcpy (buffer + bufused,
- nhost ?: "", hostlen);
- nuser = memcpy ((char *) nhost + hostlen,
- nuser ?: "", userlen);
- ndomain = memcpy ((char *) nuser + userlen,
- ndomain ?: "", domainlen);
+ if (__glibc_unlikely (newlen > buflen))
+ {
+ buflen = 2 * newlen;
+ buffer = xrealloc (buffer, buflen);
}
char *wp = buffer + buffilled;
@@ -328,8 +291,8 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
}
else if (status == NSS_STATUS_UNAVAIL && e == ERANGE)
{
- buflen *= 2;
- buffer = xrealloc (buffer, buflen);
+ tempbuflen *= 2;
+ tempbuf = xrealloc (tempbuf, tempbuflen);
}
}
@@ -474,6 +437,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
}
out:
+ free (tempbuf);
free (buffer);
*resultp = dataset;