Re: [PATCH 30/30] nss: Directly load nss_dns, without going through dlsym/dlopen

[Carlos O'Donell via Libc-alpha <libc-alpha@sourceware.org>]

On 7/8/21 11:07 AM, Florian Weimer via Libc-alpha wrote:
> This partially fixes static-only NSS support (bug 27959): The dns
> module no longer needs dlopen.  Support for the files module remains
> to be added, and also support for disabling dlopen altogher.

Noted that files is already included and commit message is adjusted.

The design here looks good, we split out files and dns and handle
them as builtins with only a handful of functions to initialize
them without too much abstraction.

OK for glibc 2.34.

Tested without regression on x86_64 and i686.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
Tested-by: Carlos O'Donell <carlos@redhat.com>
 
> This commit introduces module_load_builtin into nss/nss_module.c, which
> handles the common parts of loading the built-in nss_files and nss_dns
> modules.
> ---
>  include/nss_dns.h          | 13 +++++----
>  nss/nss_files_functions.c  |  6 -----
>  nss/nss_module.c           | 55 +++++++++++++++++++++++++++++---------
>  nss/nss_module.h           | 10 +++++--
>  resolv/Makefile            |  1 +
>  resolv/nss_dns_functions.c | 40 +++++++++++++++++++++++++++
>  6 files changed, 99 insertions(+), 26 deletions(-)
>  create mode 100644 resolv/nss_dns_functions.c
> 
> diff --git a/include/nss_dns.h b/include/nss_dns.h
> index 63b5853870..53205b27a6 100644
> --- a/include/nss_dns.h
> +++ b/include/nss_dns.h
> @@ -24,13 +24,16 @@
>  NSS_DECLARE_MODULE_FUNCTIONS (dns)
>  
>  libc_hidden_proto (_nss_dns_getcanonname_r)

> -libc_hidden_proto (_nss_dns_gethostbyname3_r)
> -libc_hidden_proto (_nss_dns_gethostbyname2_r)
> -libc_hidden_proto (_nss_dns_gethostbyname_r)
> -libc_hidden_proto (_nss_dns_gethostbyname4_r)

OK.

>  libc_hidden_proto (_nss_dns_gethostbyaddr2_r)
>  libc_hidden_proto (_nss_dns_gethostbyaddr_r)
> -libc_hidden_proto (_nss_dns_getnetbyname_r)
> +libc_hidden_proto (_nss_dns_gethostbyname2_r)
> +libc_hidden_proto (_nss_dns_gethostbyname3_r)
> +libc_hidden_proto (_nss_dns_gethostbyname4_r)
> +libc_hidden_proto (_nss_dns_gethostbyname_r)

OK. Sort.

>  libc_hidden_proto (_nss_dns_getnetbyaddr_r)
> +libc_hidden_proto (_nss_dns_getnetbyname_r)
> +
> +void __nss_dns_functions (nss_module_functions_untyped pointers)
> +  attribute_hidden;

OK.

>  
>  #endif
> diff --git a/nss/nss_files_functions.c b/nss/nss_files_functions.c
> index 85720b4311..46040fff70 100644
> --- a/nss/nss_files_functions.c
> +++ b/nss/nss_files_functions.c
> @@ -34,10 +34,4 @@ __nss_files_functions (nss_module_functions_untyped pointers)
>  #undef DEFINE_NSS_FUNCTION
>  #define DEFINE_NSS_FUNCTION(x) *fptr++ = _nss_files_##x;
>  #include "function.def"
> -
> -#ifdef PTR_MANGLE
> -  void **end = fptr;
> -  for (fptr = pointers; fptr != end; ++fptr)
> -    PTR_MANGLE (*fptr);
> -#endif

OK. No more PTR_MANGLE handling required because we are not loaded.

>  }
> diff --git a/nss/nss_module.c b/nss/nss_module.c
> index 7b42c585a4..a4a66866fb 100644
> --- a/nss/nss_module.c
> +++ b/nss/nss_module.c
> @@ -26,11 +26,13 @@
>  #include <dlfcn.h>
>  #include <gnu/lib-names.h>
>  #include <libc-lock.h>
> +#include <nss_dns.h>
> +#include <nss_files.h>

OK.

>  #include <stddef.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> -#include <nss_files.h>
> +#include <sysdep.h>
>  
>  /* Suffix after .so of NSS service modules.  This is a bit of magic,
>     but we assume LIBNSS_FILES_SO looks like "libnss_files.so.2" and we
> @@ -111,18 +113,12 @@ static const function_name nss_function_name_array[] =
>  #include "function.def"
>    };
>  
> +/* Loads a built-in module, binding the symbols using the supplied
> +   callback function.  Always returns true.  */
>  static bool
> -module_load_nss_files (struct nss_module *module)
> +module_load_builtin (struct nss_module *module,
> +		     void (*bind) (nss_module_functions_untyped))
>  {
> -  if (is_nscd)
> -    {
> -      void (*cb) (size_t, struct traced_file *) = nscd_init_cb;
> -#  ifdef PTR_DEMANGLE
> -      PTR_DEMANGLE (cb);
> -#  endif
> -      _nss_files_init (cb);
> -    }
> -
>    /* Initialize the function pointers, following the double-checked
>       locking idiom.  */
>    __libc_lock_lock (nss_module_list_lock);
> @@ -130,7 +126,13 @@ module_load_nss_files (struct nss_module *module)
>      {
>      case nss_module_uninitialized:
>      case nss_module_failed:
> -      __nss_files_functions (module->functions.untyped);
> +      bind (module->functions.untyped);
> +
> +#ifdef PTR_MANGLE
> +      for (int i = 0; i < nss_module_functions_count; ++i)
> +	PTR_MANGLE (module->functions.untyped[i]);
> +#endif
> +
>        module->handle = NULL;
>        /* Synchronizes with unlocked __nss_module_load atomic_load_acquire.  */
>        atomic_store_release (&module->state, nss_module_loaded);
> @@ -143,12 +145,37 @@ module_load_nss_files (struct nss_module *module)
>    return true;
>  }
>  
> +/* Loads the built-in nss_files module.  */
> +static bool
> +module_load_nss_files (struct nss_module *module)
> +{
> +  if (is_nscd)
> +    {
> +      void (*cb) (size_t, struct traced_file *) = nscd_init_cb;
> +#  ifdef PTR_DEMANGLE
> +      PTR_DEMANGLE (cb);
> +#  endif
> +      _nss_files_init (cb);
> +    }
> +  return module_load_builtin (module, __nss_files_functions);
> +}

OK.

> +
> +/* Loads the built-in nss_dns module.  */
> +static bool
> +module_load_nss_dns (struct nss_module *module)
> +{
> +  return module_load_builtin (module, __nss_dns_functions);
> +}

OK.

> +
> +
>  /* Internal implementation of __nss_module_load.  */
>  static bool
>  module_load (struct nss_module *module)
>  {
>    if (strcmp (module->name, "files") == 0)
>      return module_load_nss_files (module);
> +  if (strcmp (module->name, "dns") == 0)
> +    return module_load_nss_dns (module);

OK. Handle specifically.

>  
>    void *handle;
>    {
> @@ -396,7 +423,9 @@ __nss_module_freeres (void)
>    struct nss_module *current = nss_module_list;
>    while (current != NULL)
>      {
> -      if (current->state == nss_module_loaded && current->handle != NULL)
> +      /* Ignore built-in modules (which have a NULL handle).  */
> +      if (current->state == nss_module_loaded
> +	  && current->handle != NULL)

OK.

>          __libc_dlclose (current->handle);
>  
>        struct nss_module *next = current->next;
> diff --git a/nss/nss_module.h b/nss/nss_module.h
> index c1a1d90b60..b52c2935d2 100644
> --- a/nss/nss_module.h
> +++ b/nss/nss_module.h
> @@ -33,10 +33,16 @@ struct nss_module_functions
>  #include "function.def"
>  };
>  
> +/* Number of elements of the nss_module_functions_untyped array.  */
> +enum
> +  {
> +    nss_module_functions_count = (sizeof (struct nss_module_functions)
> +                                  / sizeof (void *))
> +  };

OK.

> +
>  /* Untyped version of struct nss_module_functions, for consistent
>     processing purposes.  */
> -typedef void *nss_module_functions_untyped[sizeof (struct nss_module_functions)
> -                                           / sizeof (void *)];
> +typedef void *nss_module_functions_untyped[nss_module_functions_count];

OK.

>  
>  /* Locate the nss_files functions, as if by dlopen/dlsym.  */
>  void __nss_files_functions (nss_module_functions_untyped pointers)
> diff --git a/resolv/Makefile b/resolv/Makefile
> index dd0a98c74f..31d27454b4 100644
> --- a/resolv/Makefile
> +++ b/resolv/Makefile
> @@ -48,6 +48,7 @@ routines := \
>    ns_name_unpack \
>    ns_samename \
>    nsap_addr \
> +  nss_dns_functions \

OK.

>    res-close \
>    res-name-checking \
>    res-state \
> diff --git a/resolv/nss_dns_functions.c b/resolv/nss_dns_functions.c
> new file mode 100644
> index 0000000000..158dafec90
> --- /dev/null
> +++ b/resolv/nss_dns_functions.c
> @@ -0,0 +1,40 @@
> +/* Direct access for nss_dns functions for NSS module loading.

OK.

> +   Copyright (C) 2021 Free Software Foundation, Inc.
> +   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/nss_module.h>
> +#include <nss_dns.h>
> +#include <string.h>
> +
> +void
> +__nss_dns_functions (nss_module_functions_untyped pointers)
> +{
> +  struct nss_module_functions typed =
> +    {
> +      .getcanonname_r = &_nss_dns_getcanonname_r,
> +      .gethostbyname3_r = &_nss_dns_gethostbyname3_r,
> +      .gethostbyname2_r = &_nss_dns_gethostbyname2_r,
> +      .gethostbyname_r = &_nss_dns_gethostbyname_r,
> +      .gethostbyname4_r = &_nss_dns_gethostbyname4_r,
> +      .gethostbyaddr2_r = &_nss_dns_gethostbyaddr2_r,
> +      .gethostbyaddr_r = &_nss_dns_gethostbyaddr_r,
> +      .getnetbyname_r = &_nss_dns_getnetbyname_r,
> +      .getnetbyaddr_r = &_nss_dns_getnetbyaddr_r,
> +    };
> +
> +  memcpy (pointers, &typed, sizeof (nss_module_functions_untyped));

OK.

> +}
> 


-- 
Cheers,
Carlos.