On 9/5/20 7:47 AM, Eric Blake wrote:
I noticed while reading the code that we have a documentation hole
that
may cause memory leaks for clients that are unaware, in relation to
completion callbacks.
The situation arises as follows: for all commands with a completion
callback, I checked that the code has clean semantics: either
nbd_aio_FOO() returns -1 and we never call the callback cleanup, or
nbd_aio_FOO() returns a cookie and we call the callback cleanup when the
command is retired. And since completion callbacks can only ever be
passed to nbd_aio_FOO() functions, it is simple enough to document that
when nbd_aio_FOO() fails (possible when calling them while the state
machine is in the wrong state, or with invalid flags, or a command that
the server is unwilling to accept - all things we can check client-side
without traffic to the server), then the callback function was NOT
registered, and the user must clean any resources then and there to
avoid a leak. Only when nbd_aio_FOO() succeeds will the responsibility
for cleanup be handled by the callback.
But for nbd_block_status and nbd_pread_structured, we have a second
callback. And _these_ callbacks have a problem: we can return -1 for
two different reasons, either because the command was never attempted
(nbd_aio_FOO failed) and so the callback will never be cleaned, or
because the command got sent to the server and the server failed it (the
callback WILL be cleaned). As this is ambiguous, it risks being a
memory leak. So I think we have a bug in our code, and need to
strengthen our promise of whether the cleanup callback will be called,
regardless of success or failure, while still minimizing any incompat
changes that might cause a double-free for existing clients.
Thinking about it more, it is probably easiest to just declare that we
guarantee the cleanup callback is called regardless of failure mode, for
all closures, and that we merely had a memory leak in earlier libnbd
releases. Yes, this means that we have a risk of older apps hitting a
double free if they cleaned up after a cookie of -1 to compensate for
our failure to cleanup; but it is unlikely, since the code for an app to
call the cleanup manually is more verbose, and since good apps are
unlikely to call functions that are going to fail client-side to trigger
the problem in the first place. I'll post a patch, including coverage
in tests/closure-lifetimes.c, to demonstrate what I mean.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization:
qemu.org |
libvirt.org