[1/4] nss: Introduce <nss_module.h>

  This provides the struct nss_module type, which combines the old
struct service_library type with the known_function tree, by
statically allocating space for all function pointers.

struct nss_module is fairly large (536 bytes), but it will be
shared across NSS databases.  The old known_function handling
had non-some per-function overhead (at least 32 bytes per looked-up
function, but more for long function anmes), so overall, this is not
too bad.  Resolving all functions at load time simplifies locking,
and the repeated lookups should be fast because the caches are hot
at this point.
---
 malloc/set-freeres.c |   3 +
 nss/Makefile         |   2 +-
 nss/nss_module.c     | 300 +++++++++++++++++++++++++++++++++++++++++++
 nss/nss_module.h     | 152 ++++++++++++++++++++++
 4 files changed, 456 insertions(+), 1 deletion(-)
 create mode 100644 nss/nss_module.c
 create mode 100644 nss/nss_module.h
  

On 6/25/20 12:04 AM, DJ Delorie via Libc-alpha wrote:
> This provides the struct nss_module type, which combines the old
> struct service_library type with the known_function tree, by
> statically allocating space for all function pointers.
> 
> struct nss_module is fairly large (536 bytes), but it will be
> shared across NSS databases.  The old known_function handling
> had non-some per-function overhead (at least 32 bytes per looked-up
> function, but more for long function anmes), so overall, this is not
> too bad.  Resolving all functions at load time simplifies locking,
> and the repeated lookups should be fast because the caches are hot
> at this point.

Post v2 please:
- Add comments as noted throughout.
- One code change suggestion for static array size.

> ---
>  malloc/set-freeres.c |   3 +
>  nss/Makefile         |   2 +-
>  nss/nss_module.c     | 300 +++++++++++++++++++++++++++++++++++++++++++
>  nss/nss_module.h     | 152 ++++++++++++++++++++++
>  4 files changed, 456 insertions(+), 1 deletion(-)
>  create mode 100644 nss/nss_module.c
>  create mode 100644 nss/nss_module.h
> 
> diff --git a/malloc/set-freeres.c b/malloc/set-freeres.c
> index aa80eb64b8..b328cca7c6 100644
> --- a/malloc/set-freeres.c
> +++ b/malloc/set-freeres.c
> @@ -20,6 +20,7 @@
>  #include <set-hooks.h>
>  #include <libc-internal.h>
>  
> +#include "../nss/nss_module.h"

OK. Include new module API functions.

>  #include "../libio/libioP.h"
>  
>  DEFINE_HOOK (__libc_subfreeres, (void));
> @@ -41,6 +42,8 @@ __libc_freeres (void)
>      {
>        void *const *p;
>  
> +      call_function_static_weak (__nss_module_freeres);

OK. For valgrind release the resources we have allocated.

> +
>        _IO_cleanup ();
>  
>        /* We run the resource freeing after IO cleanup.  */
> diff --git a/nss/Makefile b/nss/Makefile
> index 97bab5bb75..5d357eb51e 100644
> --- a/nss/Makefile
> +++ b/nss/Makefile
> @@ -28,7 +28,7 @@ headers			:= nss.h
>  routines		= nsswitch getnssent getnssent_r digits_dots \
>  			  valid_field valid_list_field rewrite_field \
>  			  $(addsuffix -lookup,$(databases)) \
> -			  compat-lookup nss_hash
> +			  compat-lookup nss_hash nss_module

OK. New object file.

>  
>  # These are the databases that go through nss dispatch.
>  # Caution: if you add a database here, you must add its real name
> diff --git a/nss/nss_module.c b/nss/nss_module.c
> new file mode 100644
> index 0000000000..29dc1139c6
> --- /dev/null
> +++ b/nss/nss_module.c
> @@ -0,0 +1,300 @@
> +/* Global list of NSS service modules.
> +   Copyright (c) 2020 Free Software Foundation, Inc.

OK.

> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <nss_module.h>
> +
> +#include <array_length.h>
> +#include <assert.h>
> +#include <atomic.h>
> +#include <dlfcn.h>
> +#include <gnu/lib-names.h>
> +#include <libc-lock.h>
> +#include <stddef.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +/* Suffix after .so of NSS service modules.  */
> +static const char *const __nss_shlib_revision = LIBNSS_FILES_SO + 15;

This is magic.

Suggest:

/* Start with a known built-in provided service e.g. LIBNSS_FILES_SO
   and extract the suffix after the of the NSS service modules e.g.
   "libnss_files.so[.2]" => ".2".
   We have no API to extract this except through the auto-generated
   lib-names.h and some static pointer manipulation e.g. "+ 15".  */

> +
> +/* A single-linked list used to implement a mapping from names to NSS

s/names/service names/g

> +   modules.  (Most systems only use five or so service modules, so a

s/service modules/modules/g

> +   list is sufficient here.)  Elements of this list are never freed
> +   during normal operation.  */
> +static struct nss_module *nss_module_list;
> +
> +/* Covers the list and also loading of individual NSS service
> +   modules.  */
> +__libc_lock_define (static, nss_module_list_lock);

OK.

> +
> +

Suggest:

/* Allocate the service NAME with length NAME_LENGTH.  If the service
   is already allocated in the nss_module_list cache then we return
   a pointer to the struct nss_module, otherwise we try to allocate
   a new struct nss_module entry and add it to the global nss_modules_list
   cache.  If we fail to allocate the entry we return NULL.  Failure to
   allocate the entry is always transient.  */

struct nss_module *
> +__nss_module_allocate (const char *name, size_t name_length)
> +{
> +  __libc_lock_lock (nss_module_list_lock);
> +
> +  struct nss_module *result = NULL;
> +  for (struct nss_module *p = nss_module_list; p != NULL; p = p->next)
> +    if (strncmp (p->name, name, name_length) == 0
> +        && p->name[name_length] == '\0')
> +      {
> +        /* Return the previously existing object.  */
> +        result = p;
> +        break;
> +      }
> +
> +  if (result == NULL)
> +    {
> +      /* Allocate a new list entry if the name was not found in the
> +         list.  */
> +      result = malloc (sizeof (*result) + name_length + 1);
> +      if (result != NULL)
> +        {
> +          result->state = nss_module_uninitialized;
> +          memcpy (result->name, name, name_length);
> +          result->name[name_length] = '\0';
> +          result->handle = NULL;
> +          result->next = nss_module_list;
> +          nss_module_list = result;
> +        }
> +    }
> +
> +  __libc_lock_unlock (nss_module_list_lock);
> +  return result;
> +}
> +
> +/* Long enough to store the name of any function.  */

Suggest:

/* Long enough to store the name of any function. Currently the
   longest function is "getprotobynumber_r" which is 19 including
   the NULL termiantor.  */

> +typedef char function_name[19];

Suggest:

Should we change 19 to `sizeof("getprotobynumber_r")` to be statically
determined by the compiler? This way it's clear what we're trying to do.
It also avoids needing to count characters, you just updated by copy and
pasting the new longest word.

> +
> +/* This must be lexicographically sorted and match struct
> +   nss_module_functions.  */
> +static const function_name nss_function_name_array[] =
> +  {
> +   "endaliasent",
> +   "endetherent",
> +   "endgrent",
> +   "endhostent",
> +   "endnetent",
> +   "endnetgrent",
> +   "endprotoent",
> +   "endpwent",
> +   "endrpcent",
> +   "endservent",
> +   "endsgent",
> +   "endspent",
> +   "getaliasbyname_r",
> +   "getaliasent_r",
> +   "getcanonname_r",
> +   "getetherent_r",
> +   "getgrent_r",
> +   "getgrgid_r",
> +   "getgrnam_r",
> +   "gethostbyaddr2_r",
> +   "gethostbyaddr_r",
> +   "gethostbyname2_r",
> +   "gethostbyname3_r",
> +   "gethostbyname4_r",
> +   "gethostbyname_r",
> +   "gethostent_r",
> +   "gethostton_r",
> +   "getnetbyaddr_r",
> +   "getnetbyname_r",
> +   "getnetent_r",
> +   "getnetgrent_r",
> +   "getntohost_r",
> +   "getprotobyname_r",
> +   "getprotobynumber_r",
> +   "getprotoent_r",
> +   "getpublickey",
> +   "getpwent_r",
> +   "getpwnam_r",
> +   "getpwuid_r",
> +   "getrpcbyname_r",
> +   "getrpcbynumber_r",
> +   "getrpcent_r",
> +   "getsecretkey",
> +   "getservbyname_r",
> +   "getservbyport_r",
> +   "getservent_r",
> +   "getsgent_r",
> +   "getsgnam_r",
> +   "getspent_r",
> +   "getspnam_r",
> +   "initgroups_dyn",
> +   "netname2user",
> +   "setaliasent",
> +   "setetherent",
> +   "setgrent",
> +   "sethostent",
> +   "setnetent",
> +   "setnetgrent",
> +   "setprotoent",
> +   "setpwent",
> +   "setrpcent",
> +   "setservent",
> +   "setsgent",
> +   "setspent",
> +  };
> +
> +_Static_assert ((array_length (nss_function_name_array) * sizeof (void *))
> +                == sizeof (struct nss_module_functions),
> +                "length of nss_module_name_array");
> +_Static_assert (array_length (nss_function_name_array)
> +                == array_length ((struct nss_module) { 0 }.functions.untyped),
> +                "length of nss_module_name_array");
> +

Suggest:

/* Internal implementation of __nss_module_load.  */

> +static bool
> +module_load (struct nss_module *module)
> +{
> +  void *handle;
> +  {
> +    char *shlib_name;
> +    if (__asprintf (&shlib_name, "libnss_%s.so%s",
> +                    module->name, __nss_shlib_revision) < 0)
> +      /* This is definitely a temporary failure.  Do not update
> +         module->state.  This will trigger another attempt at the next
> +         call.  */
> +      return false;
> +
> +    handle = __libc_dlopen (shlib_name);
> +    free (shlib_name);
> +  }
> +

Suggest:

/* Failing to load the module can be caused by several different scenarios.
   One such scenario is that the module has been removed from the disk.
   In which case the in-memory version is all that we have, and if the
   module->state indidates it is loaded then we can use it.  */

This gives consideration to the fact that in a long-running process we might
have seen several upgrades and changes to nsswitch.conf.

> +  if (handle == NULL)
> +    {
> +      /* dlopen failure.  We do not know if this a temporary or
> +         permanent error.  See bug 22041.  Update the state using the
> +         double-checked locking idiom.  */
> +
> +      __libc_lock_lock (nss_module_list_lock);
> +      bool result = result;
> +      switch ((enum nss_module_state) atomic_load_acquire (&module->state))
> +        {
> +        case nss_module_uninitialized:
> +          atomic_store_release (&module->state, nss_module_failed);
> +          result = false;
> +          break;
> +        case nss_module_loaded:
> +          result = true;
> +          break;
> +        case nss_module_failed:
> +          result = false;
> +          break;
> +        }
> +      __libc_lock_unlock (nss_module_list_lock);
> +      return result;
> +    }
> +
> +  nss_module_functions_untyped pointers;
> +

Suggest:
/* Copy the function pointers locally.  */

> +  for (size_t idx = 0; idx < array_length (nss_function_name_array); ++idx)
> +    {
> +      char *function_name;
> +      if (__asprintf (&function_name, "_nss_%s_%s",
> +                      module->name, nss_function_name_array[idx]) < 0)
> +        {
> +          /* Definitely a temporary error.  */
> +          __libc_dlclose (handle);
> +          return false;
> +        }
> +      pointers[idx] = __libc_dlsym (handle, function_name);

OK. Lookup and copy pointers, mangling as we do.

> +      free (function_name);
> +#ifdef PTR_MANGLE
> +      PTR_MANGLE (pointers[idx]);
> +#endif
> +    }

OK.

> +
> +  /* Intall the function pointers, following the double-checked

s/Intall/Install/g

> +     locking idiom.  Delay this after all processing, in case loading
> +     the module triggers unwinding.  */
> +  __libc_lock_lock (nss_module_list_lock);
> +  switch ((enum nss_module_state) atomic_load_acquire (&module->state))
> +    {
> +    case nss_module_uninitialized:
> +    case nss_module_failed:
> +      memcpy (module->functions.untyped, pointers,
> +              sizeof (module->functions.untyped));
> +      module->handle = handle;

Suggest:

/* Synchronizes with unlocked __nss_module_load atomic_load_acquire.  */


> +      atomic_store_release (&module->state, nss_module_loaded);
> +      break;
> +    case nss_module_loaded:

Suggest:

/* Turns out the module was already loaded, so close our own handle.
   Closing our own handle does not actually unload the modules, only
   the reference counter is decremented for the loaded module.  */

> +      /* This does not actually unload the module, only the reference
> +         counter is decremented.  */
> +      __libc_dlclose (handle);
> +      break;
> +    }
> +  __libc_lock_unlock (nss_module_list_lock);
> +  return true;
> +}
> +
> +/* Ensures that MODULE is in a loaded or failed state.  */

Suggest:

/* Force the module identified by MODULE to be loaded.  We return false
   if the module could not be loaded, true otherwise.  Loading the module
   requires looking up all the possible interface APIs and caching the
   results.  */

> +bool
> +__nss_module_load (struct nss_module *module)
> +{
> +  switch ((enum nss_module_state) atomic_load_acquire (&module->state))
> +    {
> +    case nss_module_uninitialized:
> +      return module_load (module);
> +    case nss_module_loaded:
> +      /* Loading has already succeeded.  */
> +      return true;
> +    case nss_module_failed:
> +      /* Loading previously failed.  */
> +      return false;
> +    }
> +  __builtin_unreachable ();
> +}
> +
> +static int
> +name_search (const void *left, const void *right)
> +{
> +  return strcmp (left, right);
> +}

OK.

> +

Suggest:

/* Load module MODULE and return a pointer to the module's implementation
   of NAME, otherwise return NULL on failure or error.  */

> +void *
> +__nss_module_get_function (struct nss_module *module, const char *name)
> +{
> +  if (!__nss_module_load (module))
> +    return NULL;
> +
> +  function_name *name_entry = bsearch (name, nss_function_name_array,
> +                                       array_length (nss_function_name_array),
> +                                       sizeof (function_name), name_search);
> +  assert (name_entry != NULL);
> +  size_t idx = name_entry - nss_function_name_array;
> +  void *fptr = module->functions.untyped[idx];
> +#ifdef PTR_DEMANGLE
> +  PTR_DEMANGLE (fptr);
> +#endif
> +  return fptr;
> +}
> +
> +void __libc_freeres_fn_section
> +__nss_module_freeres (void)
> +{
> +  struct nss_module *current = nss_module_list;
> +  while (current != NULL)
> +    {
> +      if (current->state == nss_module_loaded)
> +        __libc_dlclose (current->handle);

OK. Close the handle.

> +
> +      struct nss_module *next = current->next;
> +      free (current);

OK. Free the structure of the module in memory.

> +      current = next;

OK. Go to the next one.

> +    }
> +  nss_module_list = NULL;

OK. Set the nss_module_list to NULL to indicate everything is freed.

> +}
> diff --git a/nss/nss_module.h b/nss/nss_module.h
> new file mode 100644
> index 0000000000..745f99a58e
> --- /dev/null
> +++ b/nss/nss_module.h
> @@ -0,0 +1,152 @@
> +/* Global list of NSS service modules.
> +   Copyright (c) 2020 Free Software Foundation, Inc.

OK.

> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#ifndef _NSS_MODULE_H
> +#define _NSS_MODULE_H
> +
> +#include <nss.h>
> +#include <stdbool.h>
> +
> +/* Typed function pointers for all functions that can be defined by a
> +   service module.  */

OK. Nice list!

> +struct nss_module_functions
> +{
> +  nss_endaliasent *endaliasent;
> +  nss_endetherent *endetherent;
> +  nss_endgrent *endgrent;
> +  nss_endhostent *endhostent;
> +  nss_endnetent *endnetent;
> +  nss_endnetgrent *endnetgrent;
> +  nss_endprotoent *endprotoent;
> +  nss_endpwent *endpwent;
> +  nss_endrpcent *endrpcent;
> +  nss_endservent *endservent;
> +  nss_endsgent *endsgent;
> +  nss_endspent *endspent;
> +  nss_getaliasbyname_r *getaliasbyname_r;
> +  nss_getaliasent_r *getaliasent_r;
> +  nss_getcanonname_r *getcanonname_r;
> +  nss_getetherent_r *getetherent_r;
> +  nss_getgrent_r *getgrent_r;
> +  nss_getgrgid_r *getgrgid_r;
> +  nss_getgrnam_r *getgrnam_r;
> +  nss_gethostbyaddr2_r *gethostbyaddr2_r;
> +  nss_gethostbyaddr_r *gethostbyaddr_r;
> +  nss_gethostbyname2_r *gethostbyname2_r;
> +  nss_gethostbyname3_r *gethostbyname3_r;
> +  nss_gethostbyname4_r *gethostbyname4_r;
> +  nss_gethostbyname_r *gethostbyname_r;
> +  nss_gethostent_r *gethostent_r;
> +  nss_gethostton_r *gethostton_r;
> +  nss_getnetbyaddr_r *getnetbyaddr_r;
> +  nss_getnetbyname_r *getnetbyname_r;
> +  nss_getnetent_r *getnetent_r;
> +  nss_getnetgrent_r *getnetgrent_r;
> +  nss_getntohost_r *getntohost_r;
> +  nss_getprotobyname_r *getprotobyname_r;
> +  nss_getprotobynumber_r *getprotobynumber_r;
> +  nss_getprotoent_r *getprotoent_r;
> +  nss_getpublickey *getpublickey;
> +  nss_getpwent_r *getpwent_r;
> +  nss_getpwnam_r *getpwnam_r;
> +  nss_getpwuid_r *getpwuid_r;
> +  nss_getrpcbyname_r *getrpcbyname_r;
> +  nss_getrpcbynumber_r *getrpcbynumber_r;
> +  nss_getrpcent_r *getrpcent_r;
> +  nss_getsecretkey *getsecretkey;
> +  nss_getservbyname_r *getservbyname_r;
> +  nss_getservbyport_r *getservbyport_r;
> +  nss_getservent_r *getservent_r;
> +  nss_getsgent_r *getsgent_r;
> +  nss_getsgnam_r *getsgnam_r;
> +  nss_getspent_r *getspent_r;
> +  nss_getspnam_r *getspnam_r;
> +  nss_initgroups_dyn *initgroups_dyn;
> +  nss_netname2user *netname2user;
> +  nss_setaliasent *setaliasent;
> +  nss_setetherent *setetherent;
> +  nss_setgrent *setgrent;
> +  nss_sethostent *sethostent;
> +  nss_setnetent *setnetent;
> +  nss_setnetgrent *setnetgrent;
> +  nss_setprotoent *setprotoent;
> +  nss_setpwent *setpwent;
> +  nss_setrpcent *setrpcent;
> +  nss_setservent *setservent;
> +  nss_setsgent *setsgent;
> +  nss_setspent *setspent;
> +};
> +
> +/* Untyped version of struct nss_module_functions, for consistent
> +   processing purposes.  */
> +typedef void *nss_module_functions_untyped[sizeof (struct nss_module_functions)
> +                                           / sizeof (void *)];

OK. Yes, this is unfortunate, but a requirement given some of the macro
magic we have to handle this. Forcing down typed function pointers can
get problematic and messy. At least this moves in the right direction.

> +
> +/* Initialization state of a NSS module.  */
> +enum nss_module_state
> +{
> +  nss_module_uninitialized,
> +  nss_module_loaded,
> +  nss_module_failed,
> +};

OK. Confirmed this matches earlier uses. These cases are consistent
with the strategy here for:
- Uninitialized: Allocated but pointers not loaded.
- Loaded: Handle present and pointers laoded.
- Failed: Non-temporary failure happened.

> +
> +/* A NSS service module (potentially unloaded).  Client code should
> +   use the functions below.  */
> +struct nss_module
> +{
> +  /* Actual type is enum nss_module_state.  Use int due to atomic
> +     access.  Used in a double-checked locking idiom.  */
> +  int state;

OK.

> +
> +  /* The function pointers in the module.  */
> +  union
> +  {
> +    struct nss_module_functions typed;
> +    nss_module_functions_untyped untyped;
> +  } functions;

OK.

> +
> +  /* Only used for __libc_freeres unloading.  */
> +  void *handle;

OK.

> +
> +  /* The next module in the list. */
> +  struct nss_module *next;

OK.

> +
> +  /* The name of the module (as it appears in /etc/nsswitch.conf).  */
> +  char name[];

OK.

> +};
> +
> +/* Allocates the NSS module NAME (of NAME_LENGTH bytes) and places it
> +   into the global list.  If it already exists in the list, return the
> +   pre-existing module.  This does not actually load the module.
> +   Returns NULL on memory allocation failure.  */
> +struct nss_module *__nss_module_allocate (const char *name,
> +                                          size_t name_length) attribute_hidden;
> +
> +/* Ensures that MODULE is in a loaded or failed state.  */
> +bool __nss_module_load (struct nss_module *module) attribute_hidden;
> +
> +/* Ensures that MODULE is loaded and returns a pointer to the function
> +   NAME defined in it.  Returns NULL if MODULE could not be loaded, or
> +   if the function NAME is not defined in the module.  */
> +void *__nss_module_get_function (struct nss_module *module, const char *name)
> +  attribute_hidden;
> +
> +/* Called from __libc_freeres.  */
> +void __nss_module_freeres (void) attribute_hidden;
> +
> +#endif /* NSS_MODULE_H */
> 

OK.