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