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 <y...@virtuozzo.com>
>> Signed-off-by: Andrey Drobyshev <andrey.drobys...@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 :|
>
