11:56 a.m.
On Wed, Jul 12, 2023 at 03:19:59PM +0000, Tage Johansson wrote:
Hello,
While writing some tests for the Rust bindings, I discovered a
memory leak with Valgrind due to a completion callback not being
freed. More specificly, the completion callback of `aio_opt_info()`
doesn't seem to be if `aio_opt_info()` failes. In this case,
`aio_opt_info()` was called in the connected state, so it should
indeed fail, but it should perhaps also call the free function
associated with the callback.
According to libnbd(3):
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.
I wanted to see / remember exactly how this was supposed to work, so I
picked nbd_aio_pread which has a callback. Does this free the
callback in all basic error cases? The answer is yes:
int64_t
nbd_aio_pread (
struct nbd_handle *h, void *buf, size_t count, uint64_t offset,
nbd_completion_callback completion_callback, uint32_t flags
)
{
...
p = aio_pread_in_permitted_state (h);
if (unlikely (!p)) {
ret = -1;
goto out;
}
...
out:
...
FREE_CALLBACK (completion_callback);
After studying the code in lib/opt.c, I think that the situation
with
synchronous callbacks like `nbd_opt_list()` is even worse. Here the list
callback will not be freed if the internal call to `nbd_unlocked_aio_opt_list()
` failes, but it will be freed if the completion callback got an error which is
returned by `nbd_opt_list()`.
/* Issue NBD_OPT_LIST and wait for the reply. */
int
nbd_unlocked_opt_list (struct nbd_handle *h, nbd_list_callback *list)
{
struct list_helper s = { .list = *list };
nbd_list_callback l = { .callback = list_visitor, .user_data = &s };
nbd_completion_callback c = { .callback = list_complete, .user_data = &s
};
if (nbd_unlocked_aio_opt_list (h, &l, &c) == -1)
Here the list callback has not been freed.
Yes, I believe this is just a bug in the implementation of
nbd_unlocked_opt_list which should be fixed (to use FREE_CALLBACK
along this path).
return -1;
SET_CALLBACK_TO_NULL (*list);
if (wait_fo_option (h) == -1)
return -1;
if (s.err) {
But here I think that the callback has been freed.
It took me a while to figure it out, but it seems so. The callbacks
are copied to h->opt_cb.fn.list and opt_cb.fn.context and then freed
in the state machine.
set_error (s.err, "server replied with error to list
request");
return -1;
}
return s.count;
}
The problem is that if `nbd_opt_list()` returns an error, I as a
user cannot know wether I should free the data associated with the
list callback myself or if that has been done already.
It seems like it's just a missing call to FREE_CALLBACK.
Maybe I am just misunderstanding the code or the API, but if not
then I wonder how I should know when I need to free the user data
associated with a callback myself and when that is done by libnbd?
No, it should work as in the documentation, else it's a bug.
Rich.
Best regards,
Tage
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html