[PATCH 2/2] nss: handle stat failure in check_reload_and_get (BZ #28752)

[Sam James]

> On 4 Jun 2022, at 21:51, DJ Delorie <dj@redhat.com> wrote:
> 
> Sam James <sam@gentoo.org> writes:
>> Huh, it still fails, but makes a bit more sense. It's upset because
>> now we apparently we return a result despite not being started
>> up:
> 
> So back to basics...
> 
> This routine is called when we have a real getXbyY() call and need
> a config.
> 
> Our code happens after we've checked for nsswitch.conf changing.
> 
> action: FALSE = error returned to user
> action: TRUE = old configuration re-used
> action: CONT = continue to load new configuration
> 
> services[db]   inode.changed  action
> 
>  NULL         NO             CONT - we need some config
>  NULL         YES            CONT - we need some config
>  non-null     NO             CONT - we need to reload
>  non-null     YES            TRUE - use old config
> 
> So I think the patch is correct.  I'm seeing tst-reload2 fail also
> (sigh, only tested tst-reload1)
> 
> It looks like we need to initialize local->root_* even if [services] is
> NULL.  I.e. we need to run that stat always, despite deferring its
> check.
> 
> This is a little messier but I think the logic is all in the right
> order.  We might need to move the local-> if{} to before the NULL check,
> although we must load the config at least once so it should be OK.
> 

I'm glad I nearly got there -- I was worried about making the stat
call at all, but the issue is referencing the result, of course, if
it's not safe to do so.

Let me give this a whack!

> diff --git a/nss/nss_database.c b/nss/nss_database.c
> index d56c5b798d..f2ed2f2c25 100644
> --- a/nss/nss_database.c
> +++ b/nss/nss_database.c
> @@ -420,23 +420,32 @@ nss_database_check_reload_and_get (struct nss_database_state *local,
>       return true;
>     }
> 
> -  /* Before we reload, verify that "/" hasn't changed.  We assume that
> -     errors here are very unlikely, but the chance that we're entering
> -     a container is also very unlikely, so we err on the side of both
> -     very unlikely things not happening at the same time.  */
> -  if (__stat64_time64 ("/", &str) != 0
> -      || (local->root_ino != 0
> -	  && (str.st_ino != local->root_ino
> -	      ||  str.st_dev != local->root_dev)))
> +  int stat_rv = __stat64_time64 ("/", &str);
> +
> +  if (local->data.services[database_index] != NULL)
>     {
> -      /* Change detected; disable reloading and return current state.  */
> -      atomic_store_release (&local->data.reload_disabled, 1);
> -      *result = local->data.services[database_index];
> -      __libc_lock_unlock (local->lock);
> -      return true;
> +      /* Before we reload, verify that "/" hasn't changed.  We assume that
> +        errors here are very unlikely, but the chance that we're entering
> +        a container is also very unlikely, so we err on the side of both
> +        very unlikely things not happening at the same time.  */
> +      if (stat_rv != 0
> +	  || (local->root_ino != 0
> +	      && (str.st_ino != local->root_ino
> +		  ||  str.st_dev != local->root_dev)))
> +	{
> +        /* Change detected; disable reloading and return current state.  */
> +        atomic_store_release (&local->data.reload_disabled, 1);
> +        *result = local->data.services[database_index];
> +        __libc_lock_unlock (local->lock);
> +        return true;
> +      }
> +    }
> +  if (stat_rv == 0)
> +    {
> +      local->root_ino = str.st_ino;
> +      local->root_dev = str.st_dev;
>     }
> -  local->root_ino = str.st_ino;
> -  local->root_dev = str.st_dev;
> +
>   __libc_lock_unlock (local->lock);
> 
>   /* Avoid overwriting the global configuration until we have loaded
> 
> 

Best,
sam

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 358 bytes
Desc: Message signed with OpenPGP
URL: <https://sourceware.org/pipermail/libc-alpha/attachments/20220604/3fc8001d/attachment.sig>