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

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(a)redhat.com&gt; (It would be nice though if "lib/internal.h" documented the unconditional ownership transfer in nbd_internal_command_common().

Or is that clear already from another part of the documentation, or a header file?)