On Fri, 18 Dec 2015 at 16:53, Daniel P. Berrange <berra...@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 <ebl...@redhat.com> > Signed-off-by: Daniel P. Berrange <berra...@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