Re: [PATCH v2 2/7] qga: introduce ga_run_command() helper for guest cmd execution

[Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>]

On 3/5/24 19:58, Daniel P. Berrangé wrote:
> On Fri, Mar 01, 2024 at 07:28:53PM +0200, Andrey Drobyshev wrote:
>> When executing guest commands in *nix environment, we repeat the same
>> fork/exec pattern multiple times.  Let's just separate it into a single
>> helper which would also be able to feed input data into the launched
>> process' stdin.  This way we can avoid code duplication.
>>
>> To keep the history more bisectable, let's replace qmp commands
>> implementations one by one.  Also add G_GNUC_UNUSED attribute to the
>> helper and remove it in the next commit.
>>
>> Originally-by: Yuri Pudgorodskiy <yur@virtuozzo.com>
>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>> ---
>>  qga/commands-posix.c | 140 +++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 140 insertions(+)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index 8207c4c47e..781498418f 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -76,6 +76,146 @@ static void ga_wait_child(pid_t pid, int *status, Error **errp)
>>      g_assert(rpid == pid);
>>  }
>>
>> +static void ga_pipe_read_str(int fd[2], char **str, size_t *len)
>> +{
>> +    ssize_t n;
>> +    char buf[1024];
>> +    close(fd[1]);
>> +    fd[1] = -1;
>> +    while ((n = read(fd[0], buf, sizeof(buf))) != 0) {
>> +        if (n < 0) {
>> +            if (errno == EINTR) {
>> +                continue;
>> +            } else {
>> +                break;
> 
> This is a fatal error condition....
> 
>> +            }
>> +        }
>> +        *str = g_realloc(*str, *len + n);
>> +        memcpy(*str + *len, buf, n);
>> +        *len += n;
>> +    }
>> +    close(fd[0]);
>> +    fd[0] = -1;
> 
> ....and yet as far as the caller is concerned we're not indicating
> any sense of failure here. They're just being returned a partially
> read data stream as if it were successful. IMHO this needs to be
> reported properly.
>

Agreed.  We might make this helper return -errno in case of an erroneous
read from pipe, check the value in the caller and do error_setg_errno()
if smth went wrong.

> 
> Nothing in this method has NUL terminated 'str', so we are
> relying on the caller *always* honouring 'len', but.....
>

Agreed as well.  Let's allocate +1 additional byte and store '\0' in
there on every iteration, making sure our result is always
null-terminated. That way we won't need to check 'len' in the caller.

>> +}
>> +
>> +/*
>> + * Helper to run command with input/output redirection,
>> + * sending string to stdin and taking error message from
>> + * stdout/err.
>> + */
>> +G_GNUC_UNUSED
>> +static int ga_run_command(const char *argv[], const char *in_str,
>> +                          const char *action, Error **errp)
>> +{
>> +    pid_t pid;
>> +    int status;
>> +    int retcode = -1;
>> +    int infd[2] = { -1, -1 };
>> +    int outfd[2] = { -1, -1 };
>> +    char *str = NULL;
>> +    size_t len = 0;
>> +
>> +    if ((in_str && !g_unix_open_pipe(infd, FD_CLOEXEC, NULL)) ||
>> +        !g_unix_open_pipe(outfd, FD_CLOEXEC, NULL)) {
>> +        error_setg(errp, "cannot create pipe FDs");
>> +        goto out;
>> +    }
>> +
>> +    pid = fork();
>> +    if (pid == 0) {
>> +        char *cherr = NULL;
>> +
>> +        setsid();
>> +
>> +        if (in_str) {
>> +            /* Redirect stdin to infd. */
>> +            close(infd[1]);
>> +            dup2(infd[0], 0);
>> +            close(infd[0]);
>> +        } else {
>> +            reopen_fd_to_null(0);
>> +        }
>> +
>> +        /* Redirect stdout/stderr to outfd. */
>> +        close(outfd[0]);
>> +        dup2(outfd[1], 1);
>> +        dup2(outfd[1], 2);
>> +        close(outfd[1]);
>> +
>> +        execvp(argv[0], (char *const *)argv);
>> +
>> +        /* Write the cause of failed exec to pipe for the parent to read it. */
>> +        cherr = g_strdup_printf("failed to exec '%s'", argv[0]);
>> +        perror(cherr);
>> +        g_free(cherr);
>> +        _exit(EXIT_FAILURE);
>> +    } else if (pid < 0) {
>> +        error_setg_errno(errp, errno, "failed to create child process");
>> +        goto out;
>> +    }
>> +
>> +    if (in_str) {
>> +        close(infd[0]);
>> +        infd[0] = -1;
>> +        if (qemu_write_full(infd[1], in_str, strlen(in_str)) !=
>> +                strlen(in_str)) {
>> +            error_setg_errno(errp, errno, "%s: cannot write to stdin pipe",
>> +                             action);
>> +            goto out;
>> +        }
>> +        close(infd[1]);
>> +        infd[1] = -1;
>> +    }
>> +
>> +    ga_pipe_read_str(outfd, &str, &len);
>> +
>> +    ga_wait_child(pid, &status, errp);
>> +    if (*errp) {
>> +        goto out;
>> +    }
>> +
>> +    if (!WIFEXITED(status)) {
>> +        if (len) {
>> +            error_setg(errp, "child process has terminated abnormally: %s",
>> +                       str);
> 
> ...this is reading 'str' without honouring 'len', so is likely
> an array out of bounds read.
> 
>> +        } else {
>> +            error_setg(errp, "child process has terminated abnormally");
>> +        }
>> +        goto out;
>> +    }
>> +
>> +    retcode = WEXITSTATUS(status);
>> +
>> +    if (WEXITSTATUS(status)) {
>> +        if (len) {
>> +            error_setg(errp, "child process has failed to %s: %s",
>> +                       action, str);
> 
> Again, array out of bounds is likely
> 
>> +        } else {
>> +            error_setg(errp, "child process has failed to %s: exit status %d",
>> +                       action, WEXITSTATUS(status));
>> +        }
>> +        goto out;
>> +    }
>> +
>> +out:
>> +    g_free(str);
>> +
>> +    if (infd[0] != -1) {
>> +        close(infd[0]);
>> +    }
>> +    if (infd[1] != -1) {
>> +        close(infd[1]);
>> +    }
>> +    if (outfd[0] != -1) {
>> +        close(outfd[0]);
>> +    }
>> +    if (outfd[1] != -1) {
>> +        close(outfd[1]);
>> +    }
>> +
>> +    return retcode;
>> +}
>> +
>>  void qmp_guest_shutdown(const char *mode, Error **errp)
>>  {
>>      const char *shutdown_flag;
>> --
>> 2.39.3
>>
>>
> 
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>