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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
