Re: [PULL v1 3/5] qga: convert to use error checked base64 decode

[Peter Maydell]

On Fri, 18 Dec 2015 at 16:53, Daniel P. Berrange <berrange@redhat.com> wrote:
>
> Switch from using g_base64_decode over to qbase64_decode
> in order to get error checking of the base64 input data.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  qga/commands-posix.c | 11 +++++++++--
>  qga/commands-win32.c | 11 +++++++++--
>  qga/commands.c       | 13 ++++++++++++-
>  3 files changed, 30 insertions(+), 5 deletions(-)

Hi; this is an old commit, but Coverity has just noticed that
it introduced a resource leak on an error-exit codepath (CID 1460005):

> @@ -393,10 +394,19 @@ GuestExec *qmp_guest_exec(const char *path,
>      GIOChannel *in_ch, *out_ch, *err_ch;
>      GSpawnFlags flags;
>      bool has_output = (has_capture_output && capture_output);
> +    uint8_t *input = NULL;
> +    size_t ninput = 0;
>
>      arglist.value = (char *)path;
>      arglist.next = has_arg ? arg : NULL;
>
> +    if (has_input_data) {
> +        input = qbase64_decode(input_data, -1, &ninput, err);
> +        if (!input) {
> +            return NULL;
> +        }

qbase64_decode() allocates memory. We free this in the
guest_exec_child_watch callback, but moving the call to
the base64 decode function up here has put it above the call
to g_spawn_async_with_pipes(), which can fail and whose
error-exit codepath doesn't free 'input'.

> +    }
> +
>      argv = guest_exec_get_args(&arglist, true);
>      envp = has_env ? guest_exec_get_args(env, false) : NULL;
>
> @@ -425,7 +435,8 @@ GuestExec *qmp_guest_exec(const char *path,
>      g_child_watch_add(pid, guest_exec_child_watch, gei);
>
>      if (has_input_data) {
> -        gei->in.data = g_base64_decode(input_data, &gei->in.size);
> +        gei->in.data = input;
> +        gei->in.size = ninput;

The old callsite for the decode function was below the call
to g_child_watch_add(), so it could rely on the watch function
being called to free the data.

-- PMM