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>
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
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
