Re: [PATCH 07/28] elf: Implement ld.so --help

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 01/10/2020 13:32, Florian Weimer via Libc-alpha wrote:
> --help processing is deferred to the point where the executable has
> been loaded, so that it is possible to eventually include information
> from the main executable in the help output.
> 
> As suggested in the GNU command-line interface guidelines, the help
> message is printed to standard output, and the exit status is
> successful.
> 
> Handle usage errors closer to the GNU command-line interface
> guidelines.

LGTM with some nits below.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  elf/dl-main.h  |  9 +++++++--
>  elf/dl-usage.c | 24 ++++++++++++++++++----
>  elf/rtld.c     | 55 ++++++++++++++++++++++++++++++++++++++++++--------
>  3 files changed, 74 insertions(+), 14 deletions(-)
> 
> diff --git a/elf/dl-main.h b/elf/dl-main.h
> index 68dd27d0d7..71ca5114de 100644
> --- a/elf/dl-main.h
> +++ b/elf/dl-main.h
> @@ -60,7 +60,7 @@ struct audit_list
>  };
>  
>  /* This is a list of all the modes the dynamic loader can be in.  */
> -enum mode { normal, list, verify, trace };
> +enum mode { normal, list, verify, trace, rtld_help };
>  
>  /* Aggregated state information extracted from environment variables
>     and the ld.so command line.  */

Ok.

> @@ -98,6 +98,11 @@ call_init_paths (const struct dl_main_state *state)
>  }
>  
>  /* Print ld.so usage information and exit.  */
> -void _dl_usage (void) attribute_hidden __attribute__ ((__noreturn__));
> +void _dl_usage (const char *argv0, const char *wrong_option)
> +  attribute_hidden __attribute__ ((__noreturn__));
> +
> +/* Print ld.so --help output and exit.  */
> +void _dl_help (const char *argv0, struct dl_main_state *state)
> +  attribute_hidden __attribute__ ((__noreturn__));
>  
>  #endif /* _DL_MAIN */

Maybe use _Noreturn here?

> diff --git a/elf/dl-usage.c b/elf/dl-usage.c
> index f3d89d22b7..72ff99e70f 100644
> --- a/elf/dl-usage.c
> +++ b/elf/dl-usage.c
> @@ -19,12 +19,24 @@
>  #include <dl-cache.h>
>  #include <dl-main.h>
>  #include <ldsodefs.h>
> +#include <unistd.h>
>  
>  void
> -_dl_usage (void)
> +_dl_usage (const char *argv0, const char *wrong_option)
>  {
> -  _dl_fatal_printf ("\
> -Usage: ld.so [OPTION]... EXECUTABLE-FILE [ARGS-FOR-PROGRAM...]\n\
> +  if (wrong_option != NULL)
> +    _dl_error_printf ("%s: unrecognized option '%s'\n", argv0, wrong_option);
> +  else
> +    _dl_error_printf ("%s: missing program name\n", argv0);
> +  _dl_error_printf ("Try '%s --help' for more information.\n", argv0);
> +  _exit (1);
> +}
> +

Maybe EXIT_FAILURE here?

> +void
> +_dl_help (const char *argv0, struct dl_main_state *state)
> +{
> +  _dl_printf ("\
> +Usage: %s [OPTION]... EXECUTABLE-FILE [ARGS-FOR-PROGRAM...]\n\
>  You have invoked `ld.so', the helper program for shared library executables.\n\
>  This program usually lives in the file `/lib/ld.so', and special directives\n\
>  in executable files using ELF shared libraries tell the system's program\n\
> @@ -47,5 +59,9 @@ of this helper program; chances are you did not intend to run this program.\n\
>                          in LIST\n\
>    --audit LIST          use objects named in LIST as auditors\n\
>    --preload LIST        preload objects named in LIST\n\
> -  --argv0 STRING        set argv[0] to STRING before running\n");
> +  --argv0 STRING        set argv[0] to STRING before running\n\
> +  --help                display this help and exit\n\
> +",
> +              argv0);
> +  _exit (0);
>  }

Maybe EXIT_SUCCESS here?

> diff --git a/elf/rtld.c b/elf/rtld.c
> index d11fe22b83..5cdab3c99c 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -1151,6 +1151,7 @@ dl_main (const ElfW(Phdr) *phdr,
>    _dl_starting_up = 1;
>  #endif
>  
> +  const char *ld_so_name = _dl_argv[0];
>    if (*user_entry == (ElfW(Addr)) ENTRY_POINT)
>      {
>        /* Ho ho.  We are not the program interpreter!  We are the program
> @@ -1178,8 +1179,12 @@ dl_main (const ElfW(Phdr) *phdr,
>        while (_dl_argc > 1)
>  	if (! strcmp (_dl_argv[1], "--list"))
>  	  {
> -	    state.mode = list;
> -	    GLRO(dl_lazy) = -1;	/* This means do no dependency analysis.  */
> +	    if (state.mode != rtld_help)
> +	      {
> +	       state.mode = list;
> +		/* This means do no dependency analysis.  */
> +		GLRO(dl_lazy) = -1;
> +	      }
>  
>  	    ++_dl_skip_args;
>  	    --_dl_argc;

Ok.

> @@ -1187,7 +1192,8 @@ dl_main (const ElfW(Phdr) *phdr,
>  	  }
>  	else if (! strcmp (_dl_argv[1], "--verify"))
>  	  {
> -	    state.mode = verify;
> +	    if (state.mode != rtld_help)
> +	      state.mode = verify;
>  
>  	    ++_dl_skip_args;
>  	    --_dl_argc;

Ok.

> @@ -1242,13 +1248,34 @@ dl_main (const ElfW(Phdr) *phdr,
>  	    _dl_argc -= 2;
>  	    _dl_argv += 2;
>  	  }
> +	else if (strcmp (_dl_argv[1], "--help") == 0)
> +	  {
> +	    state.mode = rtld_help;
> +	    --_dl_argc;
> +	    ++_dl_argv;
> +	  }
> +	else if (_dl_argv[1][0] == '-' && _dl_argv[1][1] == '-')
> +	  {
> +	   if (_dl_argv[1][1] == '\0')
> +	     /* End of option list.  */
> +	     break;
> +	   else
> +	     /* Unrecognized option.  */
> +	     _dl_usage (ld_so_name, _dl_argv[1]);
> +	  }
>  	else
>  	  break;
>  
>        /* If we have no further argument the program was called incorrectly.
>  	 Grant the user some education.  */
>        if (_dl_argc < 2)
> -	_dl_usage ();
> +	{
> +	  if (state.mode == rtld_help)
> +	    /* --help without an executable is not an error.  */
> +	    _dl_help (ld_so_name, &state);
> +	  else
> +	    _dl_usage (ld_so_name, NULL);
> +	}
>  

Why open brackets here? There is no stack variable to possible escape.

>        ++_dl_skip_args;
>        --_dl_argc;
> @@ -1273,7 +1300,7 @@ dl_main (const ElfW(Phdr) *phdr,
>  	    break;
>  	  }
>  
> -      if (__builtin_expect (state.mode, normal) == verify)
> +      if (state.mode == verify || state.mode == rtld_help)
>  	{
>  	  const char *objname;
>  	  const char *err_str = NULL;

Ok.

> @@ -1286,9 +1313,16 @@ dl_main (const ElfW(Phdr) *phdr,
>  	  (void) _dl_catch_error (&objname, &err_str, &malloced, map_doit,
>  				  &args);
>  	  if (__glibc_unlikely (err_str != NULL))
> -	    /* We don't free the returned string, the programs stops
> -	       anyway.  */
> -	    _exit (EXIT_FAILURE);
> +	    {
> +	      /* We don't free the returned string, the programs stops
> +		 anyway.  */
> +	      if (state.mode == rtld_help)
> +		/* Mask the failure to load the main object.  The help
> +		   message contains less information in this case.  */
> +		_dl_help (ld_so_name, &state);
> +	      else
> +		_exit (EXIT_FAILURE);
> +	    }
>  	}
>        else
>  	{

Ok.

> @@ -1647,6 +1681,11 @@ dl_main (const ElfW(Phdr) *phdr,
>    audit_list_add_dynamic_tag (&state.audit_list, main_map, DT_AUDIT);
>    audit_list_add_dynamic_tag (&state.audit_list, main_map, DT_DEPAUDIT);
>  
> +  /* At this point, all data has been obtained that is included in the
> +     --help output.  */
> +  if (__builtin_expect (state.mode, normal) == rtld_help)
> +    _dl_help (ld_so_name, &state);
> +
>    /* If we have auditing DSOs to load, do it now.  */
>    bool need_security_init = true;
>    if (state.audit_list.length > 0)
> 

Use __glibc_{un}likely or just remove the branch hint (I really don't think
this matter here anyway).