Re: g_exec_* implementation

[Owen Taylor <otaylor redhat com>]

Havoc Pennington <hp@redhat.com> writes:

> It's time to play "find the bugs"!

Glad to oblige...

[...]

A couple of possible lacks:

 - I think we might need to have the environment as an (optional)
   parameter. If you are writing a security-concious program
   that needs to exec a child with a clean environment, you
   can't do that via GExecChildSetupFunc, since the only
   way to get a clean environment (as far as I know) is to
   use execve.

 - A variant of g_exec_sync that uses the main loop and returns it's
   results asynchronously to a callback might be useful.

   (Common case for this is that you want to run a program, and
   wait for completion while not blocking the main loop.)

   Something like:

 typedef void (*GExecCallback) (gchar  **standard_output,
                                gchar  **standard_error,
                                gint    *exit_status,
                                GError  *error);

 gboolean g_exec_sync_with_callback (const gchar         *working_directory,
                                     gint                argc,
                                     gchar             **argv,
                                     GExecFlags          flags,
                                     GExecChildSetupFunc child_setup,
                                     gpointer            user_data,
                                     GExecCallback       callback,
                                     GError            **error);

 (Yes, you need to pass the GError in as an in parameter since
 some errors occur during reading)

 This shouldn't be too bad to implement on top of g_exec_async().

 - It's not completely clear to me that stdin == NULL or stdin
   as a pipe is the complete set of interesting options. 

   Command line programs do sometimes want to simply let stdin
   be shared between them and their subprocesses, to let the
   subprocess do simple querying of the user. (More sophisticated 
   programs will probably use the tty interface.)

   Should there be another flag for that?

> gboolean g_exec_async (const gchar           *working_directory,
>                        gint                   argc,
>                        gchar                **argv,
>                        GExecFlags             flags,
>                        gint                  *child_pid,
>                        GExecChildSetupFunc    child_setup,
>                        gpointer               user_data,
>                        GError               **error);

child_pid should be at the end since it is an out parameter.
(Same for g_exec_async_with_pipes.)

> #define _(x) x

Hmm, I guess we should go ahead and add gettext support to glib.
(With GError, we are producing user visible errors now.)

>   /* Assuming UTF8 is already validated */

Is the intent here to actually call g_utf8_validate, or do
we simply fall over if not?
   
>   argstr = g_string_new ("");

>         {
>           /* End of an arg, append it to argv,
>            */
>           
>           argv = g_realloc (argv, (argc + 2) * sizeof(gchar*));
>           argv[argc] = g_strdup (argstr->str);
>           g_string_truncate (argstr, 0);

This results in ' foo bar' giving '' 'foo' 'bar'.

Also, you don't get 'foo ""' right (should be 'foo' '')

[...]

>             case '\\':
>               if (*cend == '\0')
>                 {
>                   g_set_error (error,
>                                G_EXEC_ERROR,
>                                G_EXEC_ERROR_PARSE,
>                                _("'\' character not allowed at the end of a command line"));
>                   g_strfreev (argv);
>                   g_string_free (argstr, TRUE);
>                   return FALSE;
>                 }

Since you don't otherwise handle '\' outside of quotes, I 
don't think you should make this check.

>   if (quote != '\0')
>     {
>       g_set_error (error,
>                    G_EXEC_ERROR,
>                    G_EXEC_ERROR_PARSE,
>                    _("Unclosed quote in command line string"));
> 
>       g_strfreev (argv);

You are missing g_string_free (argstr, TRUE) here (should move
cleanup to a 'goto error:' type cleanup block.

>   g_return_val_if_fail (argc > 0, FALSE);
>   g_return_val_if_fail (argv != NULL, FALSE);
>   g_return_val_if_fail ((flags & G_EXEC_DO_NOT_REAP_CHILD) == 0, FALSE);

I think it's generally clearer to write !(flags & G_EXEC_DO_NOT_REAP_CHILD)
since it is a check for a condition _not_ being true.

>           g_set_error (error,
>                        G_EXEC_ERROR,
>                        G_EXEC_ERROR_READ,
>                        _("Unexpected error in select() reading data from a child process (%s)"),
>                        strerror (errno));

We have g_strerror(), so you should probably use it. (I don't
know to what platforms strerror() wasn't portable, but there
apparently were some.)

>   ret = waitpid (pid, &status, 0);
>
> 
>   if (ret < 0)
>     {
>       if (errno == EINTR)
>         goto again;
>       else
>         {
>           if (!failed) /* avoid error pileups */
>             {
>               failed = TRUE;
>                   
>               g_set_error (error,
>                            G_EXEC_ERROR,
>                            G_EXEC_ERROR_READ,
>                            _("Unexpected error in waitpid() (%s)"),
>                            strerror (errno));
>             }
>         }
>     }

Using waitpid() and expecting to reliably have catch your
child exiting, unfortunately is not a completely safe assumption
in a library. 

There is not a whole lot you can do if the program is handling
SIGCHLD itself, but your probably should at least handle
things more gracefully if you get ECHILD. When exit_status
is NULL, I think you can treat this as not an error at all.

When exit_status is non-NULL, then I'm not sure if it is better
to pretend that it succeeded, pretend it succeeded but
put a g_warning ("Child disappeared"), or to return a
GError.

>       if (standard_output)        
>         {
>           g_assert (outstr);
>           *standard_output = outstr->str;
>           g_string_free (outstr, FALSE);
>         }
>       else
>         g_assert (outstr == NULL); /* no leaks! */

While I have nothing against assertions, I think this is
going a bit far on the clumsy side for something that
is quite easy to verify. 

 if (standard_output);
   *standard_output = g_string_free (outstr->str, FALSE);

is a lot clearer.

> #if 0
>     case ETXTBUSY:
>       return G_EXEC_ERROR_TXTBUSY;
>       break;
> #endif

I think it might be guaranteed to be safe to do:

#ifdef ETXTBUSY
     case ETXTBUSY:
       return G_EXEC_ERROR_TXTBUSY;
       break;
#endif
       
> static void
> set_cloexec (gint fd)
> {
>   fcntl (fd, F_SETFD, FD_CLOEXEC);
> }

As far as I know, FD_CLOEXEC is the only flag that ever can be present
here, and quite a few programs would break if that weren't the
case. But if you were _really_ paranoid, you might want to get the
flags first and OR in FD_CLOEXEC.

Most likely not worth the extra syscall.

>   if (stdin_fd >= 0)
>     {
>       /* dup2 can't actually fail here I don't think */

Actually, unix98 allows EINTR. (I have very little idea why.)
You probably should handle it.
           
>       /* Block until we get an error or EOF due
>        * to the exec() succeeding. Read a max of
>        * 128 bytes.
>        */
>       while (TRUE)
>         {
>           gint chunk;
> 
>           if (bytes >= sizeof(gint)*2)
>             break; /* give up, who knows what happened, should not be
>                     * possible.
>                     */

Actually, I think it is highly possible. Maybe you mean
bytes > sizeof(gint)*2.

[...]
           
>           if (bytes >= sizeof(gint))
>             break; /* give up, who knows what happened, should not be
>                     * possible.
>                     */

Same here. I think you need a read_int function here to cut
down on code duplication.
           
>  cleanup_and_fail:
>   close (child_err_report_pipe[0]);
>   close (child_err_report_pipe[1]);
>   close (child_pid_report_pipe[0]);
>   close (child_pid_report_pipe[1]);
>   close (stdin_pipe[0]);
>   close (stdin_pipe[1]);
>   close (stdout_pipe[0]);
>   close (stdout_pipe[1]);
>   close (stderr_pipe[0]);
>   close (stderr_pipe[1]);

Since you don't set the file descriptors to -1 on closing
them, you are creating problems here in a threaded environment.
(Another thread could get the same file descriptor and be using 
it.) 

close (-1), as you have here already, strikes me as a little
slapdash, but it should be safe.

Regards,
                                        Owen