On Wed, Jul 12, 2023 at 04:56:03PM -0500, Eric Blake wrote:
> The documentation guarantees that a user's .free callback is reached
> exactly once for all synchronous nbd_opt_* functions that take a
> callback structure (nbd_opt_list, nbd_opt_list_meta, ...), regardless
> of API success or failure, but we weren't actually testing that in the
> testsuite. It is quite easy to augment the testsuite C code to prove
> that any user's .free callback is reached exactly once, and after all
> the .callback calls (if any) have finished. We don't need to change
> the matching tests in any of the other language bindings (callbacks in
> other languages do not directly expose a .free callback to the user in
> the first place, even though the generator heavily relies on the C
> .free callback as part of creating the language bindings).
>
> The rest of this commit message is not about the patch proper, but
> about understanding why the existing code base works, since it is not
> obvious from just reading lib/opt.c. Things to note: our generated
> lib/api.c unconditionally uses FREE_CALLBACK() if the corresponding
> nbd_unlocked_ call fails for any reason, but this is a no-op if the
> code earlier transferred the callback to some other location (a
> transfer is marked by SET_CALLBACK_TO_NULL). We trigger transfer
> semantics in nbd_unlocked_aio_opt_* only when h->opt_cb is set - at
> which point, generator/states-newstyle-opt-*.c guarantees it will call
> nbd_internal_free_option() which calls the callback at the completion
> of the option, whether by successful movement back to connecting state
> (regardless of whether the server replied with success or error), or
> by transfer to the DEAD state. Meanwhile, the synchronous calls use a
> stack-local completion callback passed to the counterpart aio calls
> that kicks off the option sequence because of our use of
> {go,list,context}_complete(), and so they have to manually mark the
> user's parameter as having been transferred - but again we are
> guaranteed that once wait_for_option() returns, the state machine has
> already reached a state where our stack-local completion callback was
> reached, and therefore the user's free callback has been reached.
>
> Reported-by: Tage Johansson <[email protected]>
> Signed-off-by: Eric Blake <[email protected]>
> ---
>
> Email sent as an FYI; the patch is already pushed as commit 824de0f1
Patch looks good. I ran the tests with 'make check' and
'make check-valgrind' and couldn't see any problems.
Rich.
> tests/opt-list-meta.c | 67 ++++++++++++++++++++++++++++++++++++++-----
> tests/opt-list.c | 46 ++++++++++++++++++++++++++---
> 2 files changed, 102 insertions(+), 11 deletions(-)
>
> diff --git a/tests/opt-list-meta.c b/tests/opt-list-meta.c
> index cc67fc9b..bb731edd 100644
> --- a/tests/opt-list-meta.c
> +++ b/tests/opt-list-meta.c
> @@ -34,6 +34,7 @@
> struct progress {
> int count;
> bool seen;
> + bool freed;
> };
>
> static int
> @@ -41,12 +42,29 @@ check (void *user_data, const char *name)
> {
> struct progress *p = user_data;
>
> + if (p->freed) {
> + fprintf (stderr, "use after free callback");
> + exit (EXIT_FAILURE);
> + }
> +
> p->count++;
> if (strcmp (name, LIBNBD_CONTEXT_BASE_ALLOCATION) == 0)
> p->seen = true;
> return 0;
> }
>
> +static void
> +cb_free (void *user_data)
> +{
> + struct progress *p = user_data;
> +
> + if (p->freed) {
> + fprintf (stderr, "too many free callbacks");
> + exit (EXIT_FAILURE);
> + }
> + p->freed = true;
> +}
> +
> int
> main (int argc, char *argv[])
> {
> @@ -72,7 +90,8 @@ main (int argc, char *argv[])
> p = (struct progress) { .count = 0 };
> r = nbd_opt_list_meta_context (nbd,
> (nbd_context_callback) { .callback = check,
> - .user_data = &p});
> + .user_data = &p,
> + .free = cb_free});
> if (r == -1) {
> fprintf (stderr, "%s\n", nbd_get_error ());
> exit (EXIT_FAILURE);
> @@ -85,6 +104,10 @@ main (int argc, char *argv[])
> fprintf (stderr, "server did not reply with base:allocation\n");
> exit (EXIT_FAILURE);
> }
> + if (!p.freed) {
> + fprintf (stderr, "callback not freed by libnbd\n");
> + exit (EXIT_FAILURE);
> + }
> max = p.count;
>
> /* Second pass: bogus query has no response. */
> @@ -96,7 +119,8 @@ main (int argc, char *argv[])
> }
> r = nbd_opt_list_meta_context (nbd,
> (nbd_context_callback) { .callback = check,
> - .user_data = &p});
> + .user_data = &p,
> + .free = cb_free});
> if (r == -1) {
> fprintf (stderr, "%s\n", nbd_get_error ());
> exit (EXIT_FAILURE);
> @@ -105,6 +129,10 @@ main (int argc, char *argv[])
> fprintf (stderr, "expecting no contexts, got %d\n", r);
> exit (EXIT_FAILURE);
> }
> + if (!p.freed) {
> + fprintf (stderr, "callback not freed by libnbd\n");
> + exit (EXIT_FAILURE);
> + }
>
> /* Third pass: specific query should have one match. */
> p = (struct progress) { .count = 0 };
> @@ -129,7 +157,8 @@ main (int argc, char *argv[])
> free (tmp);
> r = nbd_opt_list_meta_context (nbd,
> (nbd_context_callback) { .callback = check,
> - .user_data = &p});
> + .user_data = &p,
> + .free = cb_free});
> if (r == -1) {
> fprintf (stderr, "%s\n", nbd_get_error ());
> exit (EXIT_FAILURE);
> @@ -138,6 +167,10 @@ main (int argc, char *argv[])
> fprintf (stderr, "expecting exactly one context, got %d\n", r);
> exit (EXIT_FAILURE);
> }
> + if (!p.freed) {
> + fprintf (stderr, "callback not freed by libnbd\n");
> + exit (EXIT_FAILURE);
> + }
>
> /* Fourth pass: nbd_opt_list_meta_context is stateless, so it should
> * not wipe status learned during nbd_opt_info().
> @@ -182,7 +215,8 @@ main (int argc, char *argv[])
> p = (struct progress) { .count = 0 };
> r = nbd_opt_list_meta_context (nbd,
> (nbd_context_callback) { .callback = check,
> - .user_data = &p});
> + .user_data = &p,
> + .free = cb_free});
> if (r == -1) {
> fprintf (stderr, "%s\n", nbd_get_error ());
> exit (EXIT_FAILURE);
> @@ -191,6 +225,10 @@ main (int argc, char *argv[])
> fprintf (stderr, "expecting no contexts, got %d\n", r);
> exit (EXIT_FAILURE);
> }
> + if (!p.freed) {
> + fprintf (stderr, "callback not freed by libnbd\n");
> + exit (EXIT_FAILURE);
> + }
> r = nbd_get_size (nbd);
> if (r == -1) {
> fprintf (stderr, "%s\n", nbd_get_error ());
> @@ -219,7 +257,8 @@ main (int argc, char *argv[])
> }
> r = nbd_opt_list_meta_context (nbd,
> (nbd_context_callback) { .callback = check,
> - .user_data = &p});
> + .user_data = &p,
> + .free = cb_free});
> if (r == -1) {
> fprintf (stderr, "%s\n", nbd_get_error ());
> exit (EXIT_FAILURE);
> @@ -228,6 +267,10 @@ main (int argc, char *argv[])
> fprintf (stderr, "expecting at least one context, got %d\n", r);
> exit (EXIT_FAILURE);
> }
> + if (!p.freed) {
> + fprintf (stderr, "callback not freed by libnbd\n");
> + exit (EXIT_FAILURE);
> + }
>
> nbd_opt_abort (nbd);
> nbd_close (nbd);
> @@ -248,7 +291,8 @@ main (int argc, char *argv[])
> p = (struct progress) { .count = 0 };
> r = nbd_opt_list_meta_context (nbd,
> (nbd_context_callback) { .callback = check,
> - .user_data = &p});
> + .user_data = &p,
> + .free = cb_free});
> if (r == -1) {
> if (nbd_stats_bytes_sent (nbd) == bytes) {
> fprintf (stderr, "bug: client failed to send request\n");
> @@ -268,6 +312,10 @@ main (int argc, char *argv[])
> exit (EXIT_FAILURE);
> }
> }
> + if (!p.freed) {
> + fprintf (stderr, "callback not freed by libnbd\n");
> + exit (EXIT_FAILURE);
> + }
>
> /* Now enable structured replies, and a retry should pass. */
> r = nbd_opt_structured_reply (nbd);
> @@ -283,7 +331,8 @@ main (int argc, char *argv[])
> p = (struct progress) { .count = 0 };
> r = nbd_opt_list_meta_context (nbd,
> (nbd_context_callback) { .callback = check,
> - .user_data = &p});
> + .user_data = &p,
> + .free = cb_free});
> if (r == -1) {
> fprintf (stderr, "%s\n", nbd_get_error ());
> exit (EXIT_FAILURE);
> @@ -297,6 +346,10 @@ main (int argc, char *argv[])
> fprintf (stderr, "server did not reply with base:allocation\n");
> exit (EXIT_FAILURE);
> }
> + if (!p.freed) {
> + fprintf (stderr, "callback not freed by libnbd\n");
> + exit (EXIT_FAILURE);
> + }
>
> nbd_opt_abort (nbd);
> nbd_close (nbd);
> diff --git a/tests/opt-list.c b/tests/opt-list.c
> index 692b292f..74a0c15e 100644
> --- a/tests/opt-list.c
> +++ b/tests/opt-list.c
> @@ -34,6 +34,7 @@
> struct progress {
> int id;
> int visit;
> + bool freed;
> };
>
> static int
> @@ -41,6 +42,11 @@ check (void *user_data, const char *name, const char
> *description)
> {
> struct progress *p = user_data;
>
> + if (p->freed) {
> + fprintf (stderr, "use after free callback");
> + exit (EXIT_FAILURE);
> + }
> +
> if (*description) {
> fprintf (stderr, "unexpected description for id %d visit %d: %s\n",
> p->id, p->visit, description);
> @@ -92,6 +98,18 @@ check (void *user_data, const char *name, const char
> *description)
> return 0;
> }
>
> +static void
> +cb_free (void *user_data)
> +{
> + struct progress *p = user_data;
> +
> + if (p->freed) {
> + fprintf (stderr, "too many free callbacks");
> + exit (EXIT_FAILURE);
> + }
> + p->freed = true;
> +}
> +
> static struct nbd_handle*
> prepare (int i)
> {
> @@ -133,7 +151,8 @@ main (int argc, char *argv[])
> nbd = prepare (0);
> p = (struct progress) { .id = 0 };
> r = nbd_opt_list (nbd, (nbd_list_callback) { .callback = check,
> - .user_data = &p});
> + .user_data = &p,
> + .free = cb_free});
> if (r != -1) {
> fprintf (stderr, "expected error after opt_list\n");
> exit (EXIT_FAILURE);
> @@ -142,13 +161,18 @@ main (int argc, char *argv[])
> fprintf (stderr, "callback called unexpectedly\n");
> exit (EXIT_FAILURE);
> }
> + if (!p.freed) {
> + fprintf (stderr, "callback not freed by libnbd\n");
> + exit (EXIT_FAILURE);
> + }
> cleanup (nbd);
>
> /* Second pass: server advertises 'a' and 'b'. */
> nbd = prepare (1);
> p = (struct progress) { .id = 1 };
> r = nbd_opt_list (nbd, (nbd_list_callback) { .callback = check,
> - .user_data = &p});
> + .user_data = &p,
> + .free = cb_free});
> if (r == -1) {
> fprintf (stderr, "%s\n", nbd_get_error ());
> exit (EXIT_FAILURE);
> @@ -158,13 +182,18 @@ main (int argc, char *argv[])
> r);
> exit (EXIT_FAILURE);
> }
> + if (!p.freed) {
> + fprintf (stderr, "callback not freed by libnbd\n");
> + exit (EXIT_FAILURE);
> + }
> cleanup (nbd);
>
> /* Third pass: server advertises empty list. */
> nbd = prepare (2);
> p = (struct progress) { .id = 2 };
> r = nbd_opt_list (nbd, (nbd_list_callback) { .callback = check,
> - .user_data = &p});
> + .user_data = &p,
> + .free = cb_free});
> if (r == -1) {
> fprintf (stderr, "%s\n", nbd_get_error ());
> exit (EXIT_FAILURE);
> @@ -174,13 +203,18 @@ main (int argc, char *argv[])
> r);
> exit (EXIT_FAILURE);
> }
> + if (!p.freed) {
> + fprintf (stderr, "callback not freed by libnbd\n");
> + exit (EXIT_FAILURE);
> + }
> cleanup (nbd);
>
> /* Final pass: server advertises 'a'. */
> nbd = prepare (3);
> p = (struct progress) { .id = 3 };
> r = nbd_opt_list (nbd, (nbd_list_callback) { .callback = check,
> - .user_data = &p});
> + .user_data = &p,
> + .free = cb_free});
> if (r == -1) {
> fprintf (stderr, "%s\n", nbd_get_error ());
> exit (EXIT_FAILURE);
> @@ -190,6 +224,10 @@ main (int argc, char *argv[])
> r);
> exit (EXIT_FAILURE);
> }
> + if (!p.freed) {
> + fprintf (stderr, "callback not freed by libnbd\n");
> + exit (EXIT_FAILURE);
> + }
> cleanup (nbd);
>
> exit (EXIT_SUCCESS);
> --
> 2.41.0
>
> _______________________________________________
> Libguestfs mailing list
> [email protected]
> https://listman.redhat.com/mailman/listinfo/libguestfs
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit
_______________________________________________
Libguestfs mailing list
[email protected]
https://listman.redhat.com/mailman/listinfo/libguestfs
