[PATCH] nss: Add missing failure check to __nss_database_get (bug 28940)

[Florian Weimer]

* DJ Delorie:

> Florian Weimer <fweimer@redhat.com> writes:
>>  __nss_database_get (enum nss_database db, nss_action_list *actions)
>>  {
>>    struct nss_database_state *local = nss_database_state_get ();
>> +  if (local == NULL)
>> +    return false;
>>    return nss_database_check_reload_and_get (local, actions, db);
>>  }
>
> This fix is obvious, but at the moment, there are two places that need
> fixing:
>
>   local = nss_database_state_get ();
>
>   result = __nss_action_parse (service_line);
>   if (result == NULL)
>     return -1;
>
>   atomic_store_release (&local->data.reload_disabled, 1);

Hmm.  You mean we should check the return value of
nss_database_state_get () there as well?

>> +/* Perform a getlogin_r call in a subprocess, to obtain the number of
>> +   allocations used and the expected result of a successful call.  */
>> +static void
>> +initialize (void *ignored)
>> +{
>> +  {
>> +    FILE *fp = fopen (_PATH_NSSWITCH_CONF, "r");
>
> As this depends (sort of) on the contents of /etc/nsswitch.conf, should
> this test be run in a container where the contents of that can be
> controlled?

I think we get more interesting results if we don't run in a container.

For example, my user is supplied by sssd.

>> +/* Perform getlogin_r in a subprocess with fault injection.  */
>> +static void
>> +test_in_subprocess (void *ignored)
>> +{
>> +  unsigned int inject_at = shared->failing_allocation;
>> +  char name[sizeof (shared->name)];
>> +  int ret = getlogin_r (name, sizeof (name));
>> +  shared->failing_allocation = ~0U;
>> +  if (ret == 0)
>> +    TEST_COMPARE_STRING (name, shared->name);
>
> So you're relying on name - an undefined string - being different than
> a result from a previous run?  I would pre-initialize name to something
> instead of leaving it undefined.

I assigned to some unlikely result string.

I tried to add coverage for the issue you pointed out, but I hit an
assertion failure:

Fatal glibc error: XXX-lookup.c:60 (__nss_passwd_lookup2): assertion failed: *ni != NULL

But for some reason reload_disabled is still set to 1:

(gdb) print *(struct nss_database_state *)global_database_state
$2 = {data = {nsswitch_conf = {size = -1, 
      ino = 15408456814510331349, mtime = {
        tv_sec = -3038287259199220267, 
        tv_nsec = -3038287259199220267}, ctime = {
        tv_sec = -3038287259199220267, 
        tv_nsec = -3038287259199220267}}, services = {0x0, 0x0, 
      0x0, 0x0, 0x0, 0x555572f73630, 0x0 <repeats 11 times>}, 
    reload_disabled = 1, initialized = false}, lock = 0, 
  root_ino = 0, root_dev = 0}

I don't quite see reason for that yet.  Because of reload_disabled, we
take the fast path.  I don't quite see how reload_disabled and
!initialized can be true at the same time.

Partial patch attached.  Any ideas?

Thanks,
Florian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-nss-Add-missing-failure-check-to-__nss_database_get-.patch
Type: text/x-patch
Size: 11805 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/libc-alpha/attachments/20250711/6461d1c8/attachment-0001.bin>