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

Reply via email to