[1.1] Clean up netgroupcache
Commit Message
On Thu, Mar 27, 2014 at 09:32:07PM +0100, Ondřej Bílka wrote:
> On Thu, Mar 27, 2014 at 08:22:54PM +0100, Ondřej Bílka wrote:
> > On Thu, Mar 27, 2014 at 03:34:11AM -0400, Mike Frysinger wrote:
> > > On Thu 27 Mar 2014 09:34:06 Siddhesh Poyarekar wrote:
> > > > Calls to stpcpy from nscd netgroups code will have overlapping source
> > > > and destination when all three values in the returned triplet are
> > > > non-NULL and in the expected (host,user,domain) order. This is seen
> > > > in valgrind as:
> > > >
> > > > Fix this by using memmove instead of stpcpy. Tested x86_64 using
> > > > various combinations of triplets (including NULL and non-NULL ones) to
> > > > verify that this works correctly and there are no regressions.
> > >
> > This could work only with additional assertion that we do not move host
> > forward otherwise it could overwrite user.
> >
> > > i feel like we've wanted an equivalent of stpcpy/memccpy for memmove. good
> > > time to add it ? :)
> > >
> > Yes, it would be better to use this at least internally, perhaps this
> > patch instead is cleaner.
> >
> > Other possibility is keep these in separate header like second snippet,
> > do you have better name for that? Also I could make a stpcat and move
> > equivalent, not sure with what name.
> >
> > Her I would fix a root cause of these bugs which is bad design. We mix
> > temporary buffer with building result. If we use separate buffers for
> > that a code would be lot simpler, I will prepare patch for it.
> >
> Here is patch that uses separate buffers.
>
Had typo there, here is fixed patch.
* nscd/netgroupcache.c: Clean up implementation.
Comments
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.
> diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c
> index 820d823..1412113 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,8 +205,7 @@ 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
> @@ -220,53 +223,11 @@ 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)
> + 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.
> {
> - 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;
> - }
> -
> - nhost = memcpy (buffer + bufused,
> - nhost ?: "", hostlen);
> - nuser = memcpy ((char *) nhost + hostlen,
> - nuser ?: "", userlen);
> - ndomain = memcpy ((char *) nuser + userlen,
> - ndomain ?: "", domainlen);
> + buflen = 2 * newlen + req->key_len;
> + buffer = xrealloc (buffer, buflen);
> }
>
> char *wp = buffer + buffilled;
> @@ -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.
> }
> }
>
> @@ -474,6 +435,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
> }
>
> out:
> + free (tempbuf);
> free (buffer);
>
> *resultp = dataset;
>
Siddhesh
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.
>
make check on x86-64, no new failures.
On Fri, Apr 11, 2014 at 04:30:53PM +0200, Ondřej Bílka wrote:
> 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.
> >
> make check on x86-64, no new failures.
make check doesn't test nscd. You'll need to test this manually by
adding various types of netgroups entries in /etc/netgroup.
Siddhesh
On Fri, Apr 11, 2014 at 10:24:15PM +0530, Siddhesh Poyarekar wrote:
> On Fri, Apr 11, 2014 at 04:30:53PM +0200, Ondřej Bílka wrote:
> > 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.
> > >
> > make check on x86-64, no new failures.
>
> make check doesn't test nscd. You'll need to test this manually by
> adding various types of netgroups entries in /etc/netgroup.
>
How, I get back to this and now study manpages which is bit confusing. I
tried to install nis for that but ypcat does not seem to work, do you
have a test script?
On Tue, May 27, 2014 at 10:27:09PM +0200, Ondřej Bílka wrote:
> > make check doesn't test nscd. You'll need to test this manually by
> > adding various types of netgroups entries in /etc/netgroup.
> >
> How, I get back to this and now study manpages which is bit confusing. I
> tried to install nis for that but ypcat does not seem to work, do you
> have a test script?
You don't need NIS to test it because you can add netgroup entries in
a file called /etc/netgroup. I don't have a script handy to create
these, but the netgroup manpage[1] should help you with the netgroup
file format to come up with one. The file format is quite simple
actually. Each netgroup entry is of type:
netgroup_name (host1, user1, domain1) (host2, user2, domain2) ...
where the stuff in brackets is called a triplet, which specifies a
combination of hostname, username and domain name that may be allowed
in a netgroup. A netgroup entry can be split into multiple line with
a trailing backslash (\). One may have wildcards in a triplet like
so:
(host1, , domain1)
This means any user on host1 and domain1. You can have similar
wildcards for hosts and domains as well, which makes all of the
following as valid triplets:
(, user, domain)
(, , domain)
(host, , domain)
(host, , )
(host, user, )
and so on.
I hope this helps.
Siddhesh
[1] http://linux.die.net/man/5/netgroup
On Wed, May 28, 2014 at 07:21:22AM +0530, Siddhesh Poyarekar wrote:
> On Tue, May 27, 2014 at 10:27:09PM +0200, Ondřej Bílka wrote:
> > > make check doesn't test nscd. You'll need to test this manually by
> > > adding various types of netgroups entries in /etc/netgroup.
> > >
> > How, I get back to this and now study manpages which is bit confusing. I
> > tried to install nis for that but ypcat does not seem to work, do you
> > have a test script?
>
> You don't need NIS to test it because you can add netgroup entries in
> a file called /etc/netgroup. I don't have a script handy to create
> these, but the netgroup manpage[1] should help you with the netgroup
> file format to come up with one. The file format is quite simple
> actually. Each netgroup entry is of type:
>
> netgroup_name (host1, user1, domain1) (host2, user2, domain2) ...
>
I already have these but do not know how to do query these, a nscd -g gives
yes cache is enabled
no cache is persistent
no cache is shared
211 suggested size
216064 total data pool size
0 used data pool size
28800 seconds time to live for positive entries
20 seconds time to live for negative entries
0 cache hits on positive entries
0 cache hits on negative entries
On Wed, May 28, 2014 at 11:05:21AM +0200, Ondřej Bílka wrote:
> I already have these but do not know how to do query these, a nscd -g gives
Ahh OK, the stats are broken. You just need to make sure that the
output you get with and without nscd is consistent.
Siddhesh
On Wed, May 28, 2014 at 04:43:18PM +0530, Siddhesh Poyarekar wrote:
> On Wed, May 28, 2014 at 11:05:21AM +0200, Ondřej Bílka wrote:
> > I already have these but do not know how to do query these, a nscd -g gives
>
> Ahh OK, the stats are broken. You just need to make sure that the
> output you get with and without nscd is consistent.
>
What output?
On Wed, May 28, 2014 at 02:20:53PM +0200, Ondřej Bílka wrote:
> On Wed, May 28, 2014 at 04:43:18PM +0530, Siddhesh Poyarekar wrote:
> > On Wed, May 28, 2014 at 11:05:21AM +0200, Ondřej Bílka wrote:
> > > I already have these but do not know how to do query these, a nscd -g gives
> >
> > Ahh OK, the stats are broken. You just need to make sure that the
> > output you get with and without nscd is consistent.
> >
> What output?
getent netgroup <netgroup-name>
On Wed, May 28, 2014 at 06:01:00PM +0530, Siddhesh Poyarekar wrote:
> On Wed, May 28, 2014 at 02:20:53PM +0200, Ondřej Bílka wrote:
> > On Wed, May 28, 2014 at 04:43:18PM +0530, Siddhesh Poyarekar wrote:
> > > On Wed, May 28, 2014 at 11:05:21AM +0200, Ondřej Bílka wrote:
> > > > I already have these but do not know how to do query these, a nscd -g gives
> > >
> > > Ahh OK, the stats are broken. You just need to make sure that the
> > > output you get with and without nscd is consistent.
> > >
> > What output?
>
> getent netgroup <netgroup-name>
Now it caches negative responses but getent netgroup group does not
write anything. /etc/netgroup that I used is following:
group (who,are,you)
powerusers (,miquels,) (,torvalds,) (,fubar,)
ourhosts (enterprise,,) (laforge,,) (Q,,)
Is that configuration problem?
On Wed, May 28, 2014 at 08:32:16PM +0200, Ondřej Bílka wrote:
> Now it caches negative responses but getent netgroup group does not
> write anything. /etc/netgroup that I used is following:
>
> group (who,are,you)
> powerusers (,miquels,) (,torvalds,) (,fubar,)
> ourhosts (enterprise,,) (laforge,,) (Q,,)
getent not writing anything to stdout implies that the netgroup was
not found. I assume you're querying with the netgroup name:
getent netgroup powerusers
which should give:
powerusers (,miquels,) (,torvalds,) (,fubar,)
Siddhesh
On Thu, May 29, 2014 at 07:28:11AM +0530, Siddhesh Poyarekar wrote:
> On Wed, May 28, 2014 at 08:32:16PM +0200, Ondřej Bílka wrote:
> > Now it caches negative responses but getent netgroup group does not
> > write anything. /etc/netgroup that I used is following:
> >
> > group (who,are,you)
> > powerusers (,miquels,) (,torvalds,) (,fubar,)
> > ourhosts (enterprise,,) (laforge,,) (Q,,)
>
> getent not writing anything to stdout implies that the netgroup was
> not found. I assume you're querying with the netgroup name:
>
> getent netgroup powerusers
>
> which should give:
>
> powerusers (,miquels,) (,torvalds,) (,fubar,)
>
Then how to configure it? It always gives not found for me.
On Thu, Jun 05, 2014 at 02:00:29PM +0200, Ondřej Bílka wrote:
> Then how to configure it? It always gives not found for me.
/etc/nsswitch.conf needs to also have 'files' as a backend.
Siddhesh
On Thu, Jun 05, 2014 at 05:38:08PM +0530, Siddhesh Poyarekar wrote:
> On Thu, Jun 05, 2014 at 02:00:29PM +0200, Ondřej Bílka wrote:
> > Then how to configure it? It always gives not found for me.
>
> /etc/nsswitch.conf needs to also have 'files' as a backend.
>
Already have that, does not work.
@@ -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,8 +205,7 @@ 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
@@ -220,53 +223,11 @@ 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)
+ size_t newlen = buffilled + hostlen + userlen + domainlen;
+ if (newlen + req->key_len > buflen)
{
- 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;
- }
-
- nhost = memcpy (buffer + bufused,
- nhost ?: "", hostlen);
- nuser = memcpy ((char *) nhost + hostlen,
- nuser ?: "", userlen);
- ndomain = memcpy ((char *) nuser + userlen,
- ndomain ?: "", domainlen);
+ buflen = 2 * newlen + req->key_len;
+ buffer = xrealloc (buffer, buflen);
}
char *wp = buffer + buffilled;
@@ -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);
}
}
@@ -474,6 +435,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
}
out:
+ free (tempbuf);
free (buffer);
*resultp = dataset;