Re: [PATCH hurd 1/6] Restructure argument setup in hashbang

[Samuel Thibault]

Applied, thanks!

Flavio Cruz, le dim. 21 janv. 2024 16:07:52 -0500, a ecrit:
> We do a few things here:
> - Move search_path to the scope where it is used to make dependencies
>   more clear.
> - Have a separate variable to store the file name we eventually need to
>   free and move the free logic to happen in a single place.
> 
> Both of this allows us to still free the name even if a fault is generated and
> also avoids a compiler warning as we try to assign a 'const char*'
> file_name_exec to a 'char *', making it more clear to what exactly we
> need to free. I also believe 'error' in line 245 was not initialized in
> case 'file_name_exec' is used and this fixes that too.
> ---
>  exec/hashexec.c | 91 +++++++++++++++++++++++--------------------------
>  1 file changed, 43 insertions(+), 48 deletions(-)
> 
> diff --git a/exec/hashexec.c b/exec/hashexec.c
> index a107291..0953679 100644
> --- a/exec/hashexec.c
> +++ b/exec/hashexec.c
> @@ -221,14 +221,14 @@ check_hashbang (struct execdata *e,
>  
>    if (! e->error)
>      {
> -      int free_file_name = 0; /* True if we should free FILE_NAME.  */
> +      char * volatile file_name_to_free = NULL; /* Will free this if set.  */
>        jmp_buf args_faulted;
>        void fault_handler (int signo)
>       { longjmp (args_faulted, 1); }
>        error_t setup_args (struct hurd_signal_preemptor *preemptor)
>       {
>         size_t namelen;
> -       char * volatile file_name = NULL;
> +       const char * volatile file_name = NULL;
>  
>         if (setjmp (args_faulted))
>           file_name = NULL;
> @@ -242,44 +242,42 @@ check_hashbang (struct execdata *e,
>                named by ARGV[0] in the `PATH' environment variable
>                might find it.  */
>  
> -           error_t error;
> -           char *name;
> -           int free_name = 0; /* True if we should free NAME. */
> -           file_t name_file;
> -           mach_port_t fileid, filefsid;
> -           ino_t fileno;
> -
> -           /* Search $PATH for NAME, opening a port NAME_FILE on it.
> -              This is encapsulated in a function so we can catch faults
> -              reading the user's environment.  */
> -           error_t search_path (struct hurd_signal_preemptor *preemptor)
> +           if (file_name_exec && file_name_exec[0] != '\0')
> +             file_name = file_name_exec;
> +           else
>               {
> -               error_t err;
> -               char *path = envz_get (envp, envplen, "PATH"), *pfxed_name;
> -
> -               if (! path)
> +               error_t error;
> +               file_t name_file;
> +               mach_port_t fileid, filefsid;
> +               ino_t fileno;
> +               char *name;
> +               /* Search $PATH for NAME, opening a port NAME_FILE on it.
> +                  This is encapsulated in a function so we can catch faults
> +                  reading the user's environment.  */
> +               error_t search_path (struct hurd_signal_preemptor *preemptor)
>                   {
> -                   const size_t len = confstr (_CS_PATH, NULL, 0);
> -                   path = alloca (len);
> -                   confstr (_CS_PATH, path, len);
> -                 }
> +                   error_t err;
> +                   char *path = envz_get (envp, envplen, "PATH"), 
> *pfxed_name;
>  
> -               err = hurd_file_name_path_lookup (user_port, user_fd, 0,
> -                                                 name, path, O_EXEC, 0,
> -                                                 &name_file, &pfxed_name);
> -               if (!err && pfxed_name)
> -                 {
> -                   name = pfxed_name;
> -                   free_name = 1;
> -                 }
> +                   if (! path)
> +                     {
> +                       const size_t len = confstr (_CS_PATH, NULL, 0);
> +                       path = alloca (len);
> +                       confstr (_CS_PATH, path, len);
> +                     }
>  
> -               return err;
> -             }
> +                   err = hurd_file_name_path_lookup (user_port, user_fd, 0,
> +                                                     name, path, O_EXEC, 0,
> +                                                     &name_file, 
> &pfxed_name);
> +                   if (!err && pfxed_name)
> +                     {
> +                       name = pfxed_name;
> +                       file_name_to_free = pfxed_name;
> +                     }
> +
> +                   return err;
> +                 }
>  
> -           if (file_name_exec && file_name_exec[0] != '\0')
> -             name = file_name_exec;
> -           else
> -             {
>                 /* Try to locate the file.  */
>                 error = io_identity (file, &fileid, &filefsid, &fileno);
>                 if (error)
> @@ -320,15 +318,10 @@ check_hashbang (struct execdata *e,
>                   }
>  
>                 mach_port_deallocate (mach_task_self (), fileid);
> -             }
>  
> -           if (!error)
> -             {
> -               file_name = name;
> -               free_file_name = free_name;
> +               if (!error)
> +                 file_name = name;
>               }
> -           else if (free_name)
> -             free (name);
>           }
>  
>         if (file_name == NULL)
> @@ -354,8 +347,9 @@ check_hashbang (struct execdata *e,
>             mach_port_mod_refs (mach_task_self (), file,
>                                 MACH_PORT_RIGHT_SEND, +1);
>  
> -           file_name = alloca (100);
> -           sprintf (file_name, "/dev/fd/%d", fd);
> +           char *fd_file_name = alloca (100);
> +           sprintf (fd_file_name, "/dev/fd/%d", fd);
> +           file_name = fd_file_name;
>           }
>  
>         /* Prepare the arguments to pass to the interpreter from the original
> @@ -374,7 +368,7 @@ check_hashbang (struct execdata *e,
>         if (new_argv == (caddr_t) -1)
>           {
>             e->error = errno;
> -           return e->error;
> +           goto end_setup_args;
>           }
>         else
>           e->error = 0;
> @@ -414,10 +408,11 @@ check_hashbang (struct execdata *e,
>             memcpy (memcpy (n, arg, arg_len) + arg_len, file_name, namelen);
>           }
>  
> -       if (free_file_name)
> -         free (file_name);
> +end_setup_args:
> +       if (file_name_to_free)
> +         free (file_name_to_free);
>  
> -       return 0;
> +       return e->error;
>       }
>  
>        /* Set up the arguments.  */
> -- 
> 2.39.2
> 
> 

-- 
Samuel
«Tiens, quand j'aurai un peu de temps et une partition libre, je crois
 que je vais essayer de remplacer mes scripts de démarrage par des
 programmes Windows lancés via Wine et binfmt_misc :-)»
-+- AGV in Guide du linuxien pervers - "J'sais pas quoi faire... (air connu)"