[Libguestfs] [libnbd PATCH 1/2] api: Avoid memory leak on certain strict_mode failures

[Laszlo Ersek]

On 11/30/21 14:20, Eric Blake wrote:
> On Tue, Nov 30, 2021 at 01:08:24PM +0100, Laszlo Ersek wrote:
>> On 11/30/21 04:15, Eric Blake wrote:
>>> Commit e57681fa ("generator: Free closures on failure", v1.5.2) makes
>>> sure that once we copy a callback out of the caller's variable, then
>>> we ensure it is cleaned up on all error paths.  But I was developing
>>> two patch series in parallel, and due to botched rebasing on my part,
>>> shortly after, commit 76bc0f0b ("api: Add STRICT_BOUNDS/ZERO_SIZE to
>>> nbd_set_strict_mode", v1.5.3) accidentally re-introduced a return -1
>>> instead of a goto err on one of its two added error checks in the
>>> common handler, and that mistake was then copy-pasted into ba86bfd1
>>> ("api: Add STRICT_ALIGN to nbd_set_strict_mode", v1.5.3).
>>>
>>> As penance for reintroducing a leak so quickly back then, I'm now
>>> adding some testsuite coverage, which fails without the rw.c changes.
>>>
>>> Fixes: 76bc0f0b
>>> Fixes: ba86bfd1
>>> ---
>>>  lib/rw.c       |  4 ++--
>>>  tests/errors.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>>>  2 files changed, 46 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/lib/rw.c b/lib/rw.c
>>> index cb25dbf..4ade750 100644
>>> --- a/lib/rw.c
>>> +++ b/lib/rw.c
>>> @@ -195,13 +195,13 @@ nbd_internal_command_common (struct nbd_handle *h,
>>>      if ((h->strict & LIBNBD_STRICT_BOUNDS) &&
>>>          (offset > h->exportsize || count > h->exportsize - offset)) {
>>>        set_error (count_err, "request out of bounds");
>>> -      return -1;
>>> +      goto err;
>>>      }
>>>
>>>      if (h->block_minimum && (h->strict & LIBNBD_STRICT_ALIGN) &&
>>>          (offset | count) & (h->block_minimum - 1)) {
>>>        set_error (EINVAL, "request is unaligned");
>>> -      return -1;
>>> +      goto err;
>>>      }
>>>    }
>>
>> One of those (rare) cases where I think that "ownership transfer on
>> error" is justified.
>>
>> Acked-by: Laszlo Ersek <lersek at redhat.com>
>>
>> (It would be nice though if "lib/internal.h" documented the
>> unconditional ownership transfer in nbd_internal_command_common().
> 
> It sort of does (in the function body, rather than as a contract at
> the declaration):
> https://gitlab.com/nbdkit/libnbd/-/blob/master/lib/rw.c#L283
> | err:
> |  /* Since we did not queue the command, we must free the callbacks. */
> 
>>
>> Or is that clear already from another part of the documentation, or a
>> header file?)
> 
> It is also in the documentation:
> https://gitlab.com/nbdkit/libnbd/-/blob/master/docs/libnbd.pod#L815
> |=head2 Callback lifetimes
> |
> |You can associate an optional free function with callbacks.  Libnbd
> |will call this function when the callback will not be called again by
> |libnbd, including in the case where the API fails.

Perfect!

> 
> So I've pushed this one as commit 76f4d5af5c1c, with a bit more
> verbosity in the commit message.
> 
> I'll backport it to stable branches and probably cut a minor release
> later today, once I have the python leak fixed as well.
> 

Thanks!
Laszlo