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 <tage.j.li...@posteo.net> > Signed-off-by: Eric Blake <ebl...@redhat.com> > --- > > 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 > Libguestfs@redhat.com > 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 Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs