Re: [Libguestfs] [libnbd PATCH] tests: Test free callback in synchronous nbd_opt_* calls

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.lists(a)posteo.net&gt; Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- Email sent as an FYI; the patch is already pushed as commit 824de0f1

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(a)redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs