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
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
