nsswitch: handle missing actions properly

  RCA: the __nss_database_get return value is nonzero on ERROR, not on
MISSING.  A separate check for MISSING is needed.  This only really
affects initgroups, since it has a fallback, so needs to know if
initgroups is missing from nsswitch.conf.  Note: it's now possible to
have a line in nsswitch.conf like this:

initgroups:

That is *not* MISSING but has an empty module list.  If this is
undesired behavior, a further "&& nip->module" is needed.  The
nss_database.c patch ensures an empty module list for an empty
nsswitch.conf list, instead of 

See also https://bugzilla.redhat.com/show_bug.cgi?id=1906066

Actual proposed commit follows:

-----
Some internal functions need to know if a database has a nonzero
list of actions; success getting the database does not guarantee
that.  Add checks for such as needed.

Skip the ":" in each nsswitch.conf line so as not to add a dummy
action libnss_:.so
  

On 12/10/20 9:20 AM, DJ Delorie via Libc-alpha wrote:
> 
> RCA: the __nss_database_get return value is nonzero on ERROR, not on
> MISSING.  A separate check for MISSING is needed.  This only really
> affects initgroups, since it has a fallback, so needs to know if
> initgroups is missing from nsswitch.conf.  Note: it's now possible to
> have a line in nsswitch.conf like this:
> 
> initgroups:
> 
> That is *not* MISSING but has an empty module list.  If this is
> undesired behavior, a further "&& nip->module" is needed.  The
> nss_database.c patch ensures an empty module list for an empty
> nsswitch.conf list, instead of
> 
> See also https://bugzilla.redhat.com/show_bug.cgi?id=1906066
> 
> Actual proposed commit follows:
> 
> -----
> Some internal functions need to know if a database has a nonzero
> list of actions; success getting the database does not guarantee
> that.  Add checks for such as needed.
> 
> Skip the ":" in each nsswitch.conf line so as not to add a dummy
> action libnss_:.so

The fix is OK but there ought to be a regression test that verifies that 
getgroups() returns the full list of supplementary groups.  I'm not sure 
if the container testing can handle this though.

Siddhesh
  

* Siddhesh Poyarekar:

> The fix is OK but there ought to be a regression test that verifies
> that getgroups() returns the full list of supplementary groups.  I'm
> not sure if the container testing can handle this though.

We can test getgrouplist, which does not change the supplementary
groups, but exercises the same NSS code in the background (via
internal_getgrouplist that is also called by initgroups).

Thanks,
Florian
  

On Dez 09 2020, DJ Delorie via Libc-alpha wrote:

> diff --git a/grp/initgroups.c b/grp/initgroups.c
> index a60ca1c395..a0a836d862 100644
> --- a/grp/initgroups.c
> +++ b/grp/initgroups.c
> @@ -72,11 +72,13 @@ internal_getgrouplist (const char *user, gid_t group, long int *size,
>  
>    nss_action_list nip;
>  
> -  if (__nss_database_get (nss_database_initgroups, &nip))
> +  if (__nss_database_get (nss_database_initgroups, &nip)
> +      && nip != NULL)
>      {
>        use_initgroups_entry = true;
>      }
> -  else if (__nss_database_get (nss_database_group, &nip))
> +  else if (__nss_database_get (nss_database_group, &nip)
> +      && nip != NULL)

Indentation is off here.

> diff --git a/nss/nsswitch.c b/nss/nsswitch.c
> index 40109c744d..921062e04f 100644
> --- a/nss/nsswitch.c
> +++ b/nss/nsswitch.c
> @@ -81,7 +81,7 @@ __nss_database_lookup2 (const char *database, const char *alternate_name,
>    if (database_names[database_id] == NULL)
>      return -1;
>  
> -  if (__nss_database_get (database_id, ni))
> +  if (__nss_database_get (database_id, ni) && *ni)

No implicit boolean coercion.

Andreas.