In recent commit of v8.8.0-41-g41eb0f446c I've suggested during
review to put both xdr_free() calls under error label, assuming
that xdr_free() accepts NULL and thus is a NOP when the control
jumps onto the label even before either of @arg or @ret was
allocated. Well, turns out, xdr_free() does no accept NULL and
thus we have to guard its call. But since @dispatcher is already
set by the time either of the variables is allocated, we can
replace the condition from 'if (dispatche)' to 'if (arg)' and 'if
(ret)'.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
src/rpc/virnetserverprogram.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/rpc/virnetserverprogram.c b/src/rpc/virnetserverprogram.c
index 212e0d5ab5..58b6a278ca 100644
--- a/src/rpc/virnetserverprogram.c
+++ b/src/rpc/virnetserverprogram.c
@@ -475,10 +475,10 @@ virNetServerProgramDispatchCall(virNetServerProgram *prog,
return virNetServerClientSendMessage(client, msg);
error:
- if (dispatcher) {
+ if (arg)
xdr_free(dispatcher->arg_filter, arg);
+ if (ret)
xdr_free(dispatcher->ret_filter, ret);
- }
/* Bad stuff (de-)serializing message, but we have an
* RPC error message we can send back to the client */
--
2.35.1
On Fri, Oct 07, 2022 at 13:10:40 +0200, Michal Privoznik wrote: > In recent commit of v8.8.0-41-g41eb0f446c I've suggested during > review to put both xdr_free() calls under error label, assuming > that xdr_free() accepts NULL and thus is a NOP when the control > jumps onto the label even before either of @arg or @ret was > allocated. Well, turns out, xdr_free() does no accept NULL and > thus we have to guard its call. But since @dispatcher is already > set by the time either of the variables is allocated, we can > replace the condition from 'if (dispatche)' to 'if (arg)' and 'if *dispatcher > (ret)'. > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com> > --- > src/rpc/virnetserverprogram.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/rpc/virnetserverprogram.c b/src/rpc/virnetserverprogram.c > index 212e0d5ab5..58b6a278ca 100644 > --- a/src/rpc/virnetserverprogram.c > +++ b/src/rpc/virnetserverprogram.c > @@ -475,10 +475,10 @@ virNetServerProgramDispatchCall(virNetServerProgram *prog, > return virNetServerClientSendMessage(client, msg); > > error: > - if (dispatcher) { > + if (arg) > xdr_free(dispatcher->arg_filter, arg); > + if (ret) > xdr_free(dispatcher->ret_filter, ret); > - } The code which was replaced by commit 41eb0f446c made it look like xdr_free should not be called on the buffer/struct unless it was passed to some of other XDR functions, but looking at the code, freeing a zeroed out buffer is safe for the few nontrivial types (xdr_string, xdr_bytes, xdr_reference). Reviewed-by: Peter Krempa <pkrempa@redhat.com>
© 2016 - 2024 Red Hat, Inc.