[bz 22342] patch: fix netgroup cache keys

[bz 22342] patch: fix netgroup cache keys

[DJ Delorie]

Functionality tested on RHEL 7.  Regression tested on Fedora 26.

	[BZ #22342]
	* nscd/netgroupcache.c (addinnetgrX): Include trailing NUL in
	key value.

diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c
index b832c9315f..2f187b208c 100644
--- a/nscd/netgroupcache.c
+++ b/nscd/netgroupcache.c
@@ -480,7 +480,7 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req,
 {
   const char *group = key;
   key = (char *) rawmemchr (key, '\0') + 1;
-  size_t group_len = key - group - 1;
+  size_t group_len = key - group;
   const char *host = *key++ ? key : NULL;
   if (host != NULL)
     key = (char *) rawmemchr (key, '\0') + 1;

Re: [bz 22342] patch: fix netgroup cache keys

[Carlos O'Donell]

On 03/01/2018 02:11 PM, DJ Delorie wrote:
> Functionality tested on RHEL 7.  Regression tested on Fedora 26.
> 
> 	[BZ #22342]
> 	* nscd/netgroupcache.c (addinnetgrX): Include trailing NUL in
> 	key value.
> 
> diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c
> index b832c9315f..2f187b208c 100644
> --- a/nscd/netgroupcache.c
> +++ b/nscd/netgroupcache.c
> @@ -480,7 +480,7 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req,
>  {
>    const char *group = key;
>    key = (char *) rawmemchr (key, '\0') + 1;
> -  size_t group_len = key - group - 1;
> +  size_t group_len = key - group;
>    const char *host = *key++ ? key : NULL;
>    if (host != NULL)
>      key = (char *) rawmemchr (key, '\0') + 1;
> 

Why is this correct?

A good submission includes a justification that the
fix is logically what is required.

We do not want to paper over a problem just by testing
that a given change makes things better.

Do we understand what the problem is?

The bug report appears to make the claim that there are
two paths in the code, one which adds to the cache with
the null included in the length, and another which doesn't,
which obviously results in a cache miss.

Is that the final case we are fixing here? Can you describe
this a bit more for the commit message?

Please post a v2 with the commit message you intend to use
please.

-- 
Cheers,
Carlos.

Re: [bz 22342] patch: fix netgroup cache keys

[DJ Delorie]

"Carlos O'Donell" <carlos@redhat.com> writes:
> Why is this correct?

As noted in the referenced BZ, this is the only key in the database that
doesn't include its trailing NUL.  In this particular case, it's
problematic because there are *two* types of entries in the netgroup
cache, and when the keys don't match the cache aging and timeouts don't
work right.

> Please post a v2 with the commit message you intend to use
> please.

Will do.

Re: [bz 22342] patch: fix netgroup cache keys

[Carlos O'Donell]

On 03/01/2018 03:21 PM, DJ Delorie wrote:
> "Carlos O'Donell" <carlos@redhat.com> writes:
>> Why is this correct?
> 
> As noted in the referenced BZ, this is the only key in the database that
> doesn't include its trailing NUL.  In this particular case, it's
> problematic because there are *two* types of entries in the netgroup
> cache, and when the keys don't match the cache aging and timeouts don't
> work right.

What are the two types of entries? How do they arrive at the database?
What is the other code path that adds the null?

You are -><- close to having what I would want for a good commit message
which explains the exact source of the problem and makes it immediately
obvious that the fix has to be correct.

>> Please post a v2 with the commit message you intend to use
>> please.
> 
> Will do.
> 


-- 
Cheers,
Carlos.

Re: [bz 22342] patch V2: fix netgroup cache keys

[DJ Delorie]

Unlike other nscd caches, the netgroup cache contains two types of
records - those for "iterate through a netgroup" (i.e. setnetgrent())
and those for "is this user in this netgroup" (i.e. innetgr()),
i.e. full and partial records.  The timeout code assumes these records
have the same key for the group name, so that the collection of records
that is "this netgroup" can be expired as a unit.

However, the keys are not the same, as the in-netgroup key is generated
by nscd rather than being passed to it from elsewhere, and is generated
without the trailing NUL.  All other keys have the trailing NUL, and as
noted in the linked BZ, debug statements confirm that two keys for the
same netgroup are added to the cache with two different lengths.

The result of this is that as records in the cache expire, the purge
code only cleans out one of the two types of entries, resulting in
stale, possibly incorrect, and possibly inconsistent cache data.

The patch simply includes the existing NUL in the computation for the
key length ('key' points to the char after the NUL, and 'group' to the
first char of the group, so 'key-group' includes the first char to the
NUL, inclusive).

	[BZ #22342]
 	* nscd/netgroupcache.c (addinnetgrX): Include trailing NUL in
 	key value.

diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c
index b832c9315f..2f187b208c 100644
--- a/nscd/netgroupcache.c
+++ b/nscd/netgroupcache.c
@@ -480,7 +480,7 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req,
 {
   const char *group = key;
   key = (char *) rawmemchr (key, '\0') + 1;
-  size_t group_len = key - group - 1;
+  size_t group_len = key - group;
   const char *host = *key++ ? key : NULL;
   if (host != NULL)
     key = (char *) rawmemchr (key, '\0') + 1;

Re: [bz 22342] patch V2: fix netgroup cache keys

[Carlos O'Donell]

On 03/01/2018 06:28 PM, DJ Delorie wrote:
> 
> Unlike other nscd caches, the netgroup cache contains two types of
> records - those for "iterate through a netgroup" (i.e. setnetgrent())
> and those for "is this user in this netgroup" (i.e. innetgr()),
> i.e. full and partial records.  The timeout code assumes these records
> have the same key for the group name, so that the collection of records
> that is "this netgroup" can be expired as a unit.
> 
> However, the keys are not the same, as the in-netgroup key is generated
> by nscd rather than being passed to it from elsewhere, and is generated
> without the trailing NUL.  All other keys have the trailing NUL, and as
> noted in the linked BZ, debug statements confirm that two keys for the
> same netgroup are added to the cache with two different lengths.
> 
> The result of this is that as records in the cache expire, the purge
> code only cleans out one of the two types of entries, resulting in
> stale, possibly incorrect, and possibly inconsistent cache data.
> 
> The patch simply includes the existing NUL in the computation for the
> key length ('key' points to the char after the NUL, and 'group' to the
> first char of the group, so 'key-group' includes the first char to the
> NUL, inclusive).

Perfect. Please commit with this explanation as the commit message.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> 	[BZ #22342]
>  	* nscd/netgroupcache.c (addinnetgrX): Include trailing NUL in
>  	key value.
> 
> diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c
> index b832c9315f..2f187b208c 100644
> --- a/nscd/netgroupcache.c
> +++ b/nscd/netgroupcache.c
> @@ -480,7 +480,7 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req,
>  {
>    const char *group = key;
>    key = (char *) rawmemchr (key, '\0') + 1;
> -  size_t group_len = key - group - 1;
> +  size_t group_len = key - group;
>    const char *host = *key++ ? key : NULL;
>    if (host != NULL)
>      key = (char *) rawmemchr (key, '\0') + 1;
> 


-- 
Cheers,
Carlos.

Re: [bz 22342] patch V2: fix netgroup cache keys

[DJ Delorie]

"Carlos O'Donell" <carlos@redhat.com> writes:
> Perfect. Please commit with this explanation as the commit message.

Done.

Re: [bz 22342] patch V2: fix netgroup cache keys

[Joseph Myers]

On Thu, 1 Mar 2018, DJ Delorie wrote:

> "Carlos O'Donell" <carlos@redhat.com> writes:
> > Perfect. Please commit with this explanation as the commit message.
> 
> Done.

If the commit fixes the bug, please resolve it as FIXED with target 
milestone set to 2.28.  (If a commit mentions a bug but does not complete 
fixing it, I strongly advise making that explicit in the commit message so 
it's obvious to readers that the bug is deliberately not marked as FIXED.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [bz 22342] patch V2: fix netgroup cache keys

[DJ Delorie]

Thanks, I keep forgetting that step.  Done :-)