Libguestfs - Sep 2020 - Re: [libnbd PATCH 1/2] generator: Refactor handling of closures in unlocked functions

As promised in my earlier thread on libnbd completion callback question.

Eric Blake (2):
  generator: Refactor handling of closures in unlocked functions
  generator: Free closures on failure

 docs/libnbd.pod           |   2 +-
 generator/C.ml            |  48 +++++++++++------
 generator/C.mli           |   1 +
 lib/debug.c               |   7 +--
 lib/opt.c                 |  31 ++++++-----
 lib/rw.c                  | 109 +++++++++++++++++++++++---------------
 tests/closure-lifetimes.c |  63 +++++++++++++++++++++-
 tests/newstyle-limited.c  |  18 ++++++-
 8 files changed, 200 insertions(+), 79 deletions(-)

-- 
2.28.0

We have a memory leak when a function with a closure exits early prior
to registering that closure through some path that will guarantee
cleanup.  The easiest way to fix it is to guarantee that once a
closure is passed into a public API, it will be cleaned regardless of
whether that API succeeds or fails.  But to avoid cleaning the closure
more than once, we need to refactor our internal handling, in order to
track when a closure has been handed off for later cleaning.  The
easiest way to do this is to pass closures by reference to all
internal functions, so that helpers in the call stack can modify the
incoming pointer rather than their own copy.

This patch is just preparatory, with no semantic change.  The public
API still passes closures by copy, but the generator then operates on
the address of that closure through all internal nbd_unlocked_*
functions, rather than making further copies through each additional
function call.
---
 generator/C.ml  | 35 ++++++++++++-----------
 generator/C.mli |  1 +
 lib/debug.c     |  6 ++--
 lib/opt.c       | 26 ++++++++---------
 lib/rw.c        | 76 ++++++++++++++++++++++++-------------------------
 5 files changed, 73 insertions(+), 71 deletions(-)

diff --git a/generator/C.ml b/generator/C.ml
index deb77fa..280b319 100644
--- a/generator/C.ml
+++ b/generator/C.ml
@@ -99,16 +99,17 @@ let rec name_of_arg = function
 | UInt64 n -> [n]

 let rec print_arg_list ?(wrap = false) ?maxcol ?handle ?types ?(parens = true)
+          ?(closure_mark) args optargs +  if parens then pr "(";
+  if wrap then
+    pr_wrap ?maxcol ','
+      (fun () -> print_arg_list' ?handle ?types ?closure_mark args
optargs)
+  else
+    print_arg_list' ?handle ?types ?closure_mark args optargs;
+  if parens then pr ")"
+
+and print_arg_list' ?(handle = false) ?(types = true) ?(closure_mark =
"")
           args optargs -  if parens then pr "(";
-  if wrap then
-    pr_wrap ?maxcol ','
-      (fun () -> print_arg_list' ?handle ?types args optargs)
-  else
-    print_arg_list' ?handle ?types args optargs;
-  if parens then pr ")"
-
-and print_arg_list' ?(handle = false) ?(types = true) args optargs    let
comma = ref false in
   if handle then (
     comma := true;
@@ -137,7 +138,7 @@ and print_arg_list' ?(handle = false) ?(types = true)
args optargs           pr "%s" len
       | Closure { cbname; cbargs } ->
          if types then pr "nbd_%s_callback " cbname;
-         pr "%s_callback" cbname
+         pr "%s%s_callback" closure_mark cbname
       | Enum (n, _) ->
          if types then pr "int ";
          pr "%s" n
@@ -179,19 +180,19 @@ and print_arg_list' ?(handle = false) ?(types = true)
args optargs        match optarg with
       | OClosure { cbname; cbargs } ->
          if types then pr "nbd_%s_callback " cbname;
-         pr "%s_callback" cbname
+         pr "%s%s_callback" closure_mark cbname
       | OFlags (n, _) ->
          if types then pr "uint32_t ";
          pr "%s" n
   ) optargs

-let print_call ?wrap ?maxcol name args optargs ret +let print_call ?wrap
?maxcol ?closure_mark name args optargs ret    pr "%s nbd_%s "
(type_of_ret ret) name;
-  print_arg_list ~handle:true ?wrap ?maxcol args optargs
+  print_arg_list ~handle:true ?wrap ?maxcol ?closure_mark args optargs

-let print_extern ?wrap name args optargs ret +let print_extern ?wrap
?closure_mark name args optargs ret    pr "extern ";
-  print_call ?wrap name args optargs ret;
+  print_call ?wrap ?closure_mark name args optargs ret;
   pr ";\n"

 let rec print_cbarg_list ?(wrap = false) ?maxcol ?types ?(parens = true)
@@ -385,7 +386,7 @@ let generate_lib_unlocked_h ()    pr "\n";
   List.iter (
     fun (name, { args; optargs; ret }) ->
-      print_extern ~wrap:true ("unlocked_" ^ name) args optargs ret
+      print_extern ~wrap:true ~closure_mark:"*"
("unlocked_" ^ name) args optargs ret
   ) handle_calls;
   pr "\n";
   pr "#endif /* LIBNBD_UNLOCKED_H */\n"
@@ -543,7 +544,7 @@ let generate_lib_api_c () 
     (* Make the call. *)
     pr "  ret = nbd_unlocked_%s " name;
-    print_arg_list ~wrap:true ~types:false ~handle:true args optargs;
+    print_arg_list ~wrap:true ~types:false ~handle:true
~closure_mark:"&" args optargs;
     pr ";\n";
     pr "\n";
     if !need_out_label then
diff --git a/generator/C.mli b/generator/C.mli
index 4f71453..a7e0412 100644
--- a/generator/C.mli
+++ b/generator/C.mli
@@ -27,6 +27,7 @@ val generate_docs_api_flag_links_pod : unit -> unit
 val generate_docs_nbd_pod : string -> API.call -> unit -> unit
 val print_arg_list : ?wrap:bool -> ?maxcol:int ->
                      ?handle:bool -> ?types:bool -> ?parens:bool ->
+                     ?closure_mark:string ->
                      API.arg list -> API.optarg list -> unit
 val print_cbarg_list : ?wrap:bool -> ?maxcol:int ->
                        ?types:bool -> ?parens:bool ->
diff --git a/lib/debug.c b/lib/debug.c
index fbedbc9..1b503d9 100644
--- a/lib/debug.c
+++ b/lib/debug.c
@@ -1,5 +1,5 @@
 /* NBD client library in userspace
- * Copyright (C) 2013-2019 Red Hat Inc.
+ * Copyright (C) 2013-2020 Red Hat Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -48,12 +48,12 @@ nbd_unlocked_clear_debug_callback (struct nbd_handle *h)

 int
 nbd_unlocked_set_debug_callback (struct nbd_handle *h,
-                                 nbd_debug_callback debug_callback)
+                                 nbd_debug_callback *debug_callback)
 {
   /* This can't fail at the moment - see implementation above. */
   nbd_unlocked_clear_debug_callback (h);

-  h->debug_callback = debug_callback;
+  h->debug_callback = *debug_callback;
   return 0;
 }

diff --git a/lib/opt.c b/lib/opt.c
index cc0c145..003ecf8 100644
--- a/lib/opt.c
+++ b/lib/opt.c
@@ -74,7 +74,7 @@ nbd_unlocked_opt_go (struct nbd_handle *h)
 {
   int err;
   nbd_completion_callback c = { .callback = go_complete, .user_data = &err
};
-  int r = nbd_unlocked_aio_opt_go (h, c);
+  int r = nbd_unlocked_aio_opt_go (h, &c);

   if (r == -1)
     return r;
@@ -96,7 +96,7 @@ nbd_unlocked_opt_info (struct nbd_handle *h)
 {
   int err;
   nbd_completion_callback c = { .callback = go_complete, .user_data = &err
};
-  int r = nbd_unlocked_aio_opt_info (h, c);
+  int r = nbd_unlocked_aio_opt_info (h, &c);

   if (r == -1)
     return r;
@@ -147,13 +147,13 @@ list_complete (void *opaque, int *err)

 /* Issue NBD_OPT_LIST and wait for the reply. */
 int
-nbd_unlocked_opt_list (struct nbd_handle *h, nbd_list_callback list)
+nbd_unlocked_opt_list (struct nbd_handle *h, nbd_list_callback *list)
 {
-  struct list_helper s = { .list = list };
+  struct list_helper s = { .list = *list };
   nbd_list_callback l = { .callback = list_visitor, .user_data = &s };
   nbd_completion_callback c = { .callback = list_complete, .user_data = &s
};

-  if (nbd_unlocked_aio_opt_list (h, l, c) == -1)
+  if (nbd_unlocked_aio_opt_list (h, &l, &c) == -1)
     return -1;

   if (wait_for_option (h) == -1)
@@ -168,10 +168,10 @@ nbd_unlocked_opt_list (struct nbd_handle *h,
nbd_list_callback list)
 /* Issue NBD_OPT_GO (or NBD_OPT_EXPORT_NAME) without waiting. */
 int
 nbd_unlocked_aio_opt_go (struct nbd_handle *h,
-                         nbd_completion_callback complete)
+                         nbd_completion_callback *complete)
 {
   h->opt_current = NBD_OPT_GO;
-  h->opt_cb.completion = complete;
+  h->opt_cb.completion = *complete;

   if (nbd_internal_run (h, cmd_issue) == -1)
     debug (h, "option queued, ignoring state machine failure");
@@ -181,7 +181,7 @@ nbd_unlocked_aio_opt_go (struct nbd_handle *h,
 /* Issue NBD_OPT_INFO without waiting. */
 int
 nbd_unlocked_aio_opt_info (struct nbd_handle *h,
-                           nbd_completion_callback complete)
+                           nbd_completion_callback *complete)
 {
   if ((h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE) == 0) {
     set_error (ENOTSUP, "server is not using fixed newstyle
protocol");
@@ -189,7 +189,7 @@ nbd_unlocked_aio_opt_info (struct nbd_handle *h,
   }

   h->opt_current = NBD_OPT_INFO;
-  h->opt_cb.completion = complete;
+  h->opt_cb.completion = *complete;

   if (nbd_internal_run (h, cmd_issue) == -1)
     debug (h, "option queued, ignoring state machine failure");
@@ -209,8 +209,8 @@ nbd_unlocked_aio_opt_abort (struct nbd_handle *h)

 /* Issue NBD_OPT_LIST without waiting. */
 int
-nbd_unlocked_aio_opt_list (struct nbd_handle *h, nbd_list_callback list,
-                           nbd_completion_callback complete)
+nbd_unlocked_aio_opt_list (struct nbd_handle *h, nbd_list_callback *list,
+                           nbd_completion_callback *complete)
 {
   if ((h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE) == 0) {
     set_error (ENOTSUP, "server is not using fixed newstyle
protocol");
@@ -218,8 +218,8 @@ nbd_unlocked_aio_opt_list (struct nbd_handle *h,
nbd_list_callback list,
   }

   assert (CALLBACK_IS_NULL (h->opt_cb.fn.list));
-  h->opt_cb.fn.list = list;
-  h->opt_cb.completion = complete;
+  h->opt_cb.fn.list = *list;
+  h->opt_cb.completion = *complete;
   h->opt_current = NBD_OPT_LIST;
   if (nbd_internal_run (h, cmd_issue) == -1)
     debug (h, "option queued, ignoring state machine failure");
diff --git a/lib/rw.c b/lib/rw.c
index 8b736d3..9f2909b 100644
--- a/lib/rw.c
+++ b/lib/rw.c
@@ -48,9 +48,9 @@ nbd_unlocked_pread (struct nbd_handle *h, void *buf,
                     size_t count, uint64_t offset, uint32_t flags)
 {
   int64_t cookie;
+  nbd_completion_callback c = NBD_NULL_COMPLETION;

-  cookie = nbd_unlocked_aio_pread (h, buf, count, offset,
-                                   NBD_NULL_COMPLETION, flags);
+  cookie = nbd_unlocked_aio_pread (h, buf, count, offset, &c, flags);
   if (cookie == -1)
     return -1;

@@ -61,15 +61,14 @@ nbd_unlocked_pread (struct nbd_handle *h, void *buf,
 int
 nbd_unlocked_pread_structured (struct nbd_handle *h, void *buf,
                                size_t count, uint64_t offset,
-                               nbd_chunk_callback chunk,
+                               nbd_chunk_callback *chunk,
                                uint32_t flags)
 {
   int64_t cookie;
+  nbd_completion_callback c = NBD_NULL_COMPLETION;

   cookie = nbd_unlocked_aio_pread_structured (h, buf, count, offset,
-                                              chunk,
-                                              NBD_NULL_COMPLETION,
-                                              flags);
+                                              chunk, &c, flags);
   if (cookie == -1)
     return -1;

@@ -82,9 +81,9 @@ nbd_unlocked_pwrite (struct nbd_handle *h, const void *buf,
                      size_t count, uint64_t offset, uint32_t flags)
 {
   int64_t cookie;
+  nbd_completion_callback c = NBD_NULL_COMPLETION;

-  cookie = nbd_unlocked_aio_pwrite (h, buf, count, offset,
-                                    NBD_NULL_COMPLETION, flags);
+  cookie = nbd_unlocked_aio_pwrite (h, buf, count, offset, &c, flags);
   if (cookie == -1)
     return -1;

@@ -96,8 +95,9 @@ int
 nbd_unlocked_flush (struct nbd_handle *h, uint32_t flags)
 {
   int64_t cookie;
+  nbd_completion_callback c = NBD_NULL_COMPLETION;

-  cookie = nbd_unlocked_aio_flush (h, NBD_NULL_COMPLETION, flags);
+  cookie = nbd_unlocked_aio_flush (h, &c, flags);
   if (cookie == -1)
     return -1;

@@ -110,9 +110,9 @@ nbd_unlocked_trim (struct nbd_handle *h,
                    uint64_t count, uint64_t offset, uint32_t flags)
 {
   int64_t cookie;
+  nbd_completion_callback c = NBD_NULL_COMPLETION;

-  cookie = nbd_unlocked_aio_trim (h, count, offset,
-                                  NBD_NULL_COMPLETION, flags);
+  cookie = nbd_unlocked_aio_trim (h, count, offset, &c, flags);
   if (cookie == -1)
     return -1;

@@ -125,9 +125,9 @@ nbd_unlocked_cache (struct nbd_handle *h,
                     uint64_t count, uint64_t offset, uint32_t flags)
 {
   int64_t cookie;
+  nbd_completion_callback c = NBD_NULL_COMPLETION;

-  cookie = nbd_unlocked_aio_cache (h, count, offset,
-                                   NBD_NULL_COMPLETION, flags);
+  cookie = nbd_unlocked_aio_cache (h, count, offset, &c, flags);
   if (cookie == -1)
     return -1;

@@ -140,9 +140,9 @@ nbd_unlocked_zero (struct nbd_handle *h,
                    uint64_t count, uint64_t offset, uint32_t flags)
 {
   int64_t cookie;
+  nbd_completion_callback c = NBD_NULL_COMPLETION;

-  cookie = nbd_unlocked_aio_zero (h, count, offset,
-                                  NBD_NULL_COMPLETION, flags);
+  cookie = nbd_unlocked_aio_zero (h, count, offset, &c, flags);
   if (cookie == -1)
     return -1;

@@ -153,13 +153,13 @@ nbd_unlocked_zero (struct nbd_handle *h,
 int
 nbd_unlocked_block_status (struct nbd_handle *h,
                            uint64_t count, uint64_t offset,
-                           nbd_extent_callback extent,
+                           nbd_extent_callback *extent,
                            uint32_t flags)
 {
   int64_t cookie;
+  nbd_completion_callback c = NBD_NULL_COMPLETION;

-  cookie = nbd_unlocked_aio_block_status (h, count, offset, extent,
-                                          NBD_NULL_COMPLETION, flags);
+  cookie = nbd_unlocked_aio_block_status (h, count, offset, extent, &c,
flags);
   if (cookie == -1)
     return -1;

@@ -262,10 +262,10 @@ nbd_internal_command_common (struct nbd_handle *h,
 int64_t
 nbd_unlocked_aio_pread (struct nbd_handle *h, void *buf,
                         size_t count, uint64_t offset,
-                        nbd_completion_callback completion,
+                        nbd_completion_callback *completion,
                         uint32_t flags)
 {
-  struct command_cb cb = { .completion = completion };
+  struct command_cb cb = { .completion = *completion };

   /* We could silently accept flag DF, but it really only makes sense
    * with callbacks, because otherwise there is no observable change
@@ -283,12 +283,12 @@ nbd_unlocked_aio_pread (struct nbd_handle *h, void *buf,
 int64_t
 nbd_unlocked_aio_pread_structured (struct nbd_handle *h, void *buf,
                                    size_t count, uint64_t offset,
-                                   nbd_chunk_callback chunk,
-                                   nbd_completion_callback completion,
+                                   nbd_chunk_callback *chunk,
+                                   nbd_completion_callback *completion,
                                    uint32_t flags)
 {
-  struct command_cb cb = { .fn.chunk = chunk,
-                           .completion = completion };
+  struct command_cb cb = { .fn.chunk = *chunk,
+                           .completion = *completion };

   if ((flags & ~LIBNBD_CMD_FLAG_DF) != 0) {
     set_error (EINVAL, "invalid flag: %" PRIu32, flags);
@@ -308,10 +308,10 @@ nbd_unlocked_aio_pread_structured (struct nbd_handle *h,
void *buf,
 int64_t
 nbd_unlocked_aio_pwrite (struct nbd_handle *h, const void *buf,
                          size_t count, uint64_t offset,
-                         nbd_completion_callback completion,
+                         nbd_completion_callback *completion,
                          uint32_t flags)
 {
-  struct command_cb cb = { .completion = completion };
+  struct command_cb cb = { .completion = *completion };

   if (nbd_unlocked_is_read_only (h) == 1) {
     set_error (EPERM, "server does not support write operations");
@@ -335,10 +335,10 @@ nbd_unlocked_aio_pwrite (struct nbd_handle *h, const void
*buf,

 int64_t
 nbd_unlocked_aio_flush (struct nbd_handle *h,
-                        nbd_completion_callback completion,
+                        nbd_completion_callback *completion,
                         uint32_t flags)
 {
-  struct command_cb cb = { .completion = completion };
+  struct command_cb cb = { .completion = *completion };

   if (nbd_unlocked_can_flush (h) != 1) {
     set_error (EINVAL, "server does not support flush operations");
@@ -357,10 +357,10 @@ nbd_unlocked_aio_flush (struct nbd_handle *h,
 int64_t
 nbd_unlocked_aio_trim (struct nbd_handle *h,
                        uint64_t count, uint64_t offset,
-                       nbd_completion_callback completion,
+                       nbd_completion_callback *completion,
                        uint32_t flags)
 {
-  struct command_cb cb = { .completion = completion };
+  struct command_cb cb = { .completion = *completion };

   if (nbd_unlocked_can_trim (h) != 1) {
     set_error (EINVAL, "server does not support trim operations");
@@ -394,10 +394,10 @@ nbd_unlocked_aio_trim (struct nbd_handle *h,
 int64_t
 nbd_unlocked_aio_cache (struct nbd_handle *h,
                         uint64_t count, uint64_t offset,
-                        nbd_completion_callback completion,
+                        nbd_completion_callback *completion,
                         uint32_t flags)
 {
-  struct command_cb cb = { .completion = completion };
+  struct command_cb cb = { .completion = *completion };

   /* Actually according to the NBD protocol document, servers do exist
    * that support NBD_CMD_CACHE but don't advertise the
@@ -420,10 +420,10 @@ nbd_unlocked_aio_cache (struct nbd_handle *h,
 int64_t
 nbd_unlocked_aio_zero (struct nbd_handle *h,
                        uint64_t count, uint64_t offset,
-                       nbd_completion_callback completion,
+                       nbd_completion_callback *completion,
                        uint32_t flags)
 {
-  struct command_cb cb = { .completion = completion };
+  struct command_cb cb = { .completion = *completion };

   if (nbd_unlocked_can_zero (h) != 1) {
     set_error (EINVAL, "server does not support zero operations");
@@ -464,12 +464,12 @@ nbd_unlocked_aio_zero (struct nbd_handle *h,
 int64_t
 nbd_unlocked_aio_block_status (struct nbd_handle *h,
                                uint64_t count, uint64_t offset,
-                               nbd_extent_callback extent,
-                               nbd_completion_callback completion,
+                               nbd_extent_callback *extent,
+                               nbd_completion_callback *completion,
                                uint32_t flags)
 {
-  struct command_cb cb = { .fn.extent = extent,
-                           .completion = completion };
+  struct command_cb cb = { .fn.extent = *extent,
+                           .completion = *completion };

   if (!h->structured_replies) {
     set_error (ENOTSUP, "server does not support structured
replies");
-- 
2.28.0

We can easily demonstrate a memory leak when repeatedly calling
nbd_aio_pread from nbdsh in the wrong state.  True, that isn't
something a sane client would normally do, but it's worth fixing.

The culprit: when nbd_aio_pread returns a cookie, we guarantee to
clean up the closure, but if we fail early and never scheduled the
command, nothing ever cleans up the closure.  We could document this
as intentional, and force the user to always clean up their closure on
failure.  However, this is not nice, for two reasons.  First, a
documentation change to codify existing practice would put more burden
on clients - every client that calls a C API with closures is now on
the hook to add boilerplate to avoid a memory leak; and the fact that
we can easily demonstrate the memory leak in nbdsh means this solution
does not scale.  Second, consider nbd_pread_structured: that call
takes a chunk closure parameter, and can fail for multiple reasons,
either because it was called in the wrong state (in which case we were
not freeing the closure) or because it succeeded at getting a server
reponse where the server failed (in which case the closure is freed by
virtue of getting the server response).  As the caller can't
distinguish which, telling them to free on failure could result in
double-frees.

So the best plan of attack is to document that ALL functions that take
a closure argument promise to eventually free the closure, even if the
API fails, and to declare our memory leak as a bug to be fixed in
libnbd proper.  To do that, the generator will now blindly free
anything that has not been cleared by the unlocked_* helper, which in
turn are now careful to clear any callback once it has successfully
been copied into somewhere that will guarantee subsequent cleanup
(whether as part of the state machine, or by the fact that
nbd_internal_command_common now cleans up on all error paths).

Yes, our change means that any client that WAS checking for nbd_aio_*
returning -1 for cookies and manually cleaning up is now performing a
double free, but as argued above, such clients are unlikely because of
the extra boilerplate it would entail, and because of the fact that
most clients are sane enough to not trigger error paths that can fail
client-side without queueing up a trip to the server (for example, no
one intentionally writes a client that calls nbd_aio_pread before
nbd_connect_*, except as part of our testsuite in errors.c), to have
noticed the problem under valgrind before now.
---
 docs/libnbd.pod           |  2 +-
 generator/C.ml            | 13 ++++++++
 lib/debug.c               |  1 +
 lib/opt.c                 |  5 ++++
 lib/rw.c                  | 33 ++++++++++++++++----
 tests/closure-lifetimes.c | 63 ++++++++++++++++++++++++++++++++++++++-
 tests/newstyle-limited.c  | 18 ++++++++++-
 7 files changed, 127 insertions(+), 8 deletions(-)

diff --git a/docs/libnbd.pod b/docs/libnbd.pod
index 2c3742c..f2ba3bb 100644
--- a/docs/libnbd.pod
+++ b/docs/libnbd.pod
@@ -773,7 +773,7 @@ because you can use closures to achieve the same effect.

 You can associate an optional free function with callbacks.  Libnbd
 will call this function when the callback will not be called again by
-libnbd.
+libnbd, including in the case where the API fails.

 This can be used to free associated C<user_data>.  For example:

diff --git a/generator/C.ml b/generator/C.ml
index 280b319..4bdcf60 100644
--- a/generator/C.ml
+++ b/generator/C.ml
@@ -553,6 +553,19 @@ let generate_lib_api_c ()        print_trace_leave ret;
       pr "\n"
     );
+    (* Finish any closures not transferred to state machine. *)
+    List.iter (
+      function
+      | Closure { cbname } ->
+         pr "  FREE_CALLBACK (%s_callback);\n" cbname
+      | _ -> ()
+    ) args;
+    List.iter (
+      function
+      | OClosure { cbname } ->
+         pr "  FREE_CALLBACK (%s_callback);\n" cbname
+      | OFlags _ -> ()
+    ) optargs;
     if is_locked then (
       pr "  if (h->public_state != get_next_state (h))\n";
       pr "    h->public_state = get_next_state (h);\n";
diff --git a/lib/debug.c b/lib/debug.c
index 1b503d9..b598ad3 100644
--- a/lib/debug.c
+++ b/lib/debug.c
@@ -54,6 +54,7 @@ nbd_unlocked_set_debug_callback (struct nbd_handle *h,
   nbd_unlocked_clear_debug_callback (h);

   h->debug_callback = *debug_callback;
+  SET_CALLBACK_TO_NULL (*debug_callback);
   return 0;
 }

diff --git a/lib/opt.c b/lib/opt.c
index 003ecf8..6ea8326 100644
--- a/lib/opt.c
+++ b/lib/opt.c
@@ -156,6 +156,7 @@ nbd_unlocked_opt_list (struct nbd_handle *h,
nbd_list_callback *list)
   if (nbd_unlocked_aio_opt_list (h, &l, &c) == -1)
     return -1;

+  SET_CALLBACK_TO_NULL (*list);
   if (wait_for_option (h) == -1)
     return -1;
   if (s.err) {
@@ -172,6 +173,7 @@ nbd_unlocked_aio_opt_go (struct nbd_handle *h,
 {
   h->opt_current = NBD_OPT_GO;
   h->opt_cb.completion = *complete;
+  SET_CALLBACK_TO_NULL (*complete);

   if (nbd_internal_run (h, cmd_issue) == -1)
     debug (h, "option queued, ignoring state machine failure");
@@ -190,6 +192,7 @@ nbd_unlocked_aio_opt_info (struct nbd_handle *h,

   h->opt_current = NBD_OPT_INFO;
   h->opt_cb.completion = *complete;
+  SET_CALLBACK_TO_NULL (*complete);

   if (nbd_internal_run (h, cmd_issue) == -1)
     debug (h, "option queued, ignoring state machine failure");
@@ -219,7 +222,9 @@ nbd_unlocked_aio_opt_list (struct nbd_handle *h,
nbd_list_callback *list,

   assert (CALLBACK_IS_NULL (h->opt_cb.fn.list));
   h->opt_cb.fn.list = *list;
+  SET_CALLBACK_TO_NULL (*list);
   h->opt_cb.completion = *complete;
+  SET_CALLBACK_TO_NULL (*complete);
   h->opt_current = NBD_OPT_LIST;
   if (nbd_internal_run (h, cmd_issue) == -1)
     debug (h, "option queued, ignoring state machine failure");
diff --git a/lib/rw.c b/lib/rw.c
index 9f2909b..4b8eeaf 100644
--- a/lib/rw.c
+++ b/lib/rw.c
@@ -72,6 +72,7 @@ nbd_unlocked_pread_structured (struct nbd_handle *h, void
*buf,
   if (cookie == -1)
     return -1;

+  assert (CALLBACK_IS_NULL (*chunk));
   return wait_for_command (h, cookie);
 }

@@ -163,6 +164,7 @@ nbd_unlocked_block_status (struct nbd_handle *h,
   if (cookie == -1)
     return -1;

+  assert (CALLBACK_IS_NULL (*extent));
   return wait_for_command (h, cookie);
 }

@@ -176,11 +178,11 @@ nbd_internal_command_common (struct nbd_handle *h,

   if (h->disconnect_request) {
       set_error (EINVAL, "cannot request more commands after
NBD_CMD_DISC");
-      return -1;
+      goto err;;
   }
   if (h->in_flight == INT_MAX) {
       set_error (ENOMEM, "too many commands already in flight");
-      return -1;
+      goto err;
   }

   switch (type) {
@@ -190,7 +192,7 @@ nbd_internal_command_common (struct nbd_handle *h,
     if (count > MAX_REQUEST_SIZE) {
       set_error (ERANGE, "request too large: maximum request size is
%d",
                  MAX_REQUEST_SIZE);
-      return -1;
+      goto err;
     }
     break;

@@ -203,7 +205,7 @@ nbd_internal_command_common (struct nbd_handle *h,
     if (count > UINT32_MAX) {
       set_error (ERANGE, "request too large: maximum request size is
%" PRIu32,
                  UINT32_MAX);
-      return -1;
+      goto err;
     }
     break;
   }
@@ -211,7 +213,7 @@ nbd_internal_command_common (struct nbd_handle *h,
   cmd = calloc (1, sizeof *cmd);
   if (cmd == NULL) {
     set_error (errno, "calloc");
-    return -1;
+    goto err;
   }
   cmd->flags = flags;
   cmd->type = type;
@@ -257,6 +259,17 @@ nbd_internal_command_common (struct nbd_handle *h,
   }

   return cmd->cookie;
+
+ err:
+  /* Since we did not queue the command, we must free the callbacks. */
+  if (cb) {
+    if (type == NBD_CMD_BLOCK_STATUS)
+      FREE_CALLBACK (cb->fn.extent);
+    if (type == NBD_CMD_READ)
+      FREE_CALLBACK (cb->fn.chunk);
+    FREE_CALLBACK (cb->completion);
+  }
+  return -1;
 }

 int64_t
@@ -276,6 +289,7 @@ nbd_unlocked_aio_pread (struct nbd_handle *h, void *buf,
     return -1;
   }

+  SET_CALLBACK_TO_NULL (*completion);
   return nbd_internal_command_common (h, 0, NBD_CMD_READ, offset, count,
                                       buf, &cb);
 }
@@ -301,6 +315,8 @@ nbd_unlocked_aio_pread_structured (struct nbd_handle *h,
void *buf,
     return -1;
   }

+  SET_CALLBACK_TO_NULL (*chunk);
+  SET_CALLBACK_TO_NULL (*completion);
   return nbd_internal_command_common (h, flags, NBD_CMD_READ, offset, count,
                                       buf, &cb);
 }
@@ -329,6 +345,7 @@ nbd_unlocked_aio_pwrite (struct nbd_handle *h, const void
*buf,
     return -1;
   }

+  SET_CALLBACK_TO_NULL (*completion);
   return nbd_internal_command_common (h, flags, NBD_CMD_WRITE, offset, count,
                                       (void *) buf, &cb);
 }
@@ -350,6 +367,7 @@ nbd_unlocked_aio_flush (struct nbd_handle *h,
     return -1;
   }

+  SET_CALLBACK_TO_NULL (*completion);
   return nbd_internal_command_common (h, 0, NBD_CMD_FLUSH, 0, 0,
                                       NULL, &cb);
 }
@@ -387,6 +405,7 @@ nbd_unlocked_aio_trim (struct nbd_handle *h,
     return -1;
   }

+  SET_CALLBACK_TO_NULL (*completion);
   return nbd_internal_command_common (h, flags, NBD_CMD_TRIM, offset, count,
                                       NULL, &cb);
 }
@@ -413,6 +432,7 @@ nbd_unlocked_aio_cache (struct nbd_handle *h,
     return -1;
   }

+  SET_CALLBACK_TO_NULL (*completion);
   return nbd_internal_command_common (h, 0, NBD_CMD_CACHE, offset, count,
                                       NULL, &cb);
 }
@@ -457,6 +477,7 @@ nbd_unlocked_aio_zero (struct nbd_handle *h,
     return -1;
   }

+  SET_CALLBACK_TO_NULL (*completion);
   return nbd_internal_command_common (h, flags, NBD_CMD_WRITE_ZEROES, offset,
                                       count, NULL, &cb);
 }
@@ -493,6 +514,8 @@ nbd_unlocked_aio_block_status (struct nbd_handle *h,
     return -1;
   }

+  SET_CALLBACK_TO_NULL (*extent);
+  SET_CALLBACK_TO_NULL (*completion);
   return nbd_internal_command_common (h, flags, NBD_CMD_BLOCK_STATUS, offset,
                                       count, NULL, &cb);
 }
diff --git a/tests/closure-lifetimes.c b/tests/closure-lifetimes.c
index 70788b6..a7a1e46 100644
--- a/tests/closure-lifetimes.c
+++ b/tests/closure-lifetimes.c
@@ -1,5 +1,5 @@
 /* NBD client library in userspace
- * Copyright (C) 2013-2019 Red Hat Inc.
+ * Copyright (C) 2013-2020 Red Hat Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -42,6 +42,8 @@ static unsigned debug_fn_called;
 static unsigned debug_fn_freed;
 static unsigned read_cb_called;
 static unsigned read_cb_freed;
+static unsigned block_status_cb_called;
+static unsigned block_status_cb_freed;
 static unsigned completion_cb_called;
 static unsigned completion_cb_freed;

@@ -74,6 +76,21 @@ read_cb_free (void *opaque)
   read_cb_freed++;
 }

+static int
+block_status_cb (void *opaque, const char *meta, uint64_t offset,
+                 uint32_t *entries, size_t nr_entries, int *error)
+{
+  assert (!block_status_cb_freed);
+  block_status_cb_called++;
+  return 0;
+}
+
+static void
+block_status_cb_free (void *opaque)
+{
+  read_cb_freed++;
+}
+
 static int
 completion_cb (void *opaque, int *error)
 {
@@ -168,5 +185,49 @@ main (int argc, char *argv[])
   assert (read_cb_freed == 1);
   assert (completion_cb_freed == 1);

+  /* Test command callbacks are freed if the command fails client-side,
+   * whether from calling in wrong state or because of no server support.
+   */
+  block_status_cb_called = block_status_cb_freed +    completion_cb_called =
completion_cb_freed = 0;
+  nbd = nbd_create ();
+  if (nbd == NULL) NBD_ERROR;
+  /* Intentionally omit a call to:
+   *  nbd_add_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION);
+   */
+  cookie = nbd_aio_block_status (nbd, sizeof buf, 0,
+                                 (nbd_extent_callback) { .callback =
block_status_cb,
+                                                         .free =
block_status_cb_free },
+                                 (nbd_completion_callback) { .callback =
completion_cb,
+                                                             .free =
completion_cb_free },
+                                 0);
+  if (cookie != -1) {
+    fprintf (stderr, "%s: Expecting block_status failure\n",
argv[0]);
+    exit (EXIT_FAILURE);
+  }
+  assert (block_status_cb_freed == 1);
+  assert (completion_cb_freed == 1);
+
+  block_status_cb_called = block_status_cb_freed +    completion_cb_called =
completion_cb_freed = 0;
+
+  if (nbd_connect_command (nbd, nbdkit) == -1) NBD_ERROR;
+
+  cookie = nbd_aio_block_status (nbd, sizeof buf, 0,
+                                 (nbd_extent_callback) { .callback =
block_status_cb,
+                                                         .free =
block_status_cb_free },
+                                 (nbd_completion_callback) { .callback =
completion_cb,
+                                                             .free =
completion_cb_free },
+                                 0);
+  if (cookie != -1) {
+    fprintf (stderr, "%s: Expecting block_status failure\n",
argv[0]);
+    exit (EXIT_FAILURE);
+  }
+  assert (block_status_cb_freed == 1);
+  assert (completion_cb_freed == 1);
+
+  nbd_kill_subprocess (nbd, 0);
+  nbd_close (nbd);
+
   exit (EXIT_SUCCESS);
 }
diff --git a/tests/newstyle-limited.c b/tests/newstyle-limited.c
index fd89620..4479a14 100644
--- a/tests/newstyle-limited.c
+++ b/tests/newstyle-limited.c
@@ -84,6 +84,17 @@ list_cb (void *opaque, const char *name, const char
*description)
   exit (EXIT_FAILURE);
 }

+static bool list_freed = false;
+static void
+free_list_cb (void *opaque)
+{
+  if (list_freed) {
+    fprintf (stderr, "%s: list callback mistakenly freed twice",
progname);
+    exit (EXIT_FAILURE);
+  }
+  list_freed = true;
+}
+
 int
 main (int argc, char *argv[])
 {
@@ -144,10 +155,15 @@ main (int argc, char *argv[])
     fprintf (stderr, "unexpected state after negotiating\n");
     exit (EXIT_FAILURE);
   }
-  if (nbd_opt_list (nbd, (nbd_list_callback) { .callback = list_cb }) != -1) {
+  if (nbd_opt_list (nbd, (nbd_list_callback) { .callback = list_cb,
+                                               .free = free_list_cb }) != -1) {
     fprintf (stderr, "nbd_opt_list: expected failure\n");
     exit (EXIT_FAILURE);
   }
+  if (!list_freed) {
+    fprintf (stderr, "nbd_opt_list: list closure memory leak\n");
+    exit (EXIT_FAILURE);
+  }
   if (nbd_get_errno () != ENOTSUP) {
     fprintf (stderr, "%s: wrong errno value after failed opt_list\n",
argv[0]);
     exit (EXIT_FAILURE);
-- 
2.28.0

On Mon, Sep 07, 2020 at 04:46:39PM -0500, Eric Blake
wrote:
> We have a memory leak when a function with a closure exits early prior
> to registering that closure through some path that will guarantee
> cleanup.  The easiest way to fix it is to guarantee that once a
> closure is passed into a public API, it will be cleaned regardless of
> whether that API succeeds or fails.  But to avoid cleaning the closure
> more than once, we need to refactor our internal handling, in order to
> track when a closure has been handed off for later cleaning.  The
> easiest way to do this is to pass closures by reference to all
> internal functions, so that helpers in the call stack can modify the
> incoming pointer rather than their own copy.
> 
> This patch is just preparatory, with no semantic change.  The public
> API still passes closures by copy, but the generator then operates on
> the address of that closure through all internal nbd_unlocked_*
> functions, rather than making further copies through each additional
> function call.
> ---
>  generator/C.ml  | 35 ++++++++++++-----------
>  generator/C.mli |  1 +
>  lib/debug.c     |  6 ++--
>  lib/opt.c       | 26 ++++++++---------
>  lib/rw.c        | 76 ++++++++++++++++++++++++-------------------------
>  5 files changed, 73 insertions(+), 71 deletions(-)
> 
> diff --git a/generator/C.ml b/generator/C.ml
> index deb77fa..280b319 100644
> --- a/generator/C.ml
> +++ b/generator/C.ml
> @@ -99,16 +99,17 @@ let rec name_of_arg = function
>  | UInt64 n -> [n]
> 
>  let rec print_arg_list ?(wrap = false) ?maxcol ?handle ?types ?(parens =
true)
> +          ?(closure_mark) args optargs 
You don't need parens around here, you can just write ?closure_mark
> +  if parens then pr "(";
> +  if wrap then
> +    pr_wrap ?maxcol ','
> +      (fun () -> print_arg_list' ?handle ?types ?closure_mark args
optargs)
> +  else
> +    print_arg_list' ?handle ?types ?closure_mark args optargs;
> +  if parens then pr ")"
> +
> +and print_arg_list' ?(handle = false) ?(types = true) ?(closure_mark =
"")
...
>            args optargs >        | Closure { cbname; cbargs } ->
>           if types then pr "nbd_%s_callback " cbname;
> -         pr "%s_callback" cbname
> +         pr "%s%s_callback" closure_mark cbname
Perhaps make it type safe?

  type closure_mark = AddressOf | Dereference

And then:

  pr "%s%c_callback"
     (match closure_mark with AddressOf -> '&' | Dereference
-> '*')
     cbname

...
> @@ -385,7 +386,7 @@ let generate_lib_unlocked_h () >    pr
"\n";
>    List.iter (
>      fun (name, { args; optargs; ret }) ->
> -      print_extern ~wrap:true ("unlocked_" ^ name) args optargs
ret
> +      print_extern ~wrap:true ~closure_mark:"*"
("unlocked_" ^ name) args optargs ret
And at call sites like this one you'd use ~closure_mark:Dereference

Rest is fine.

ACK but definitely remove the superfluous parentheses above.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

On Mon, Sep 07, 2020 at 04:46:40PM -0500, Eric Blake
wrote:
> We can easily demonstrate a memory leak when repeatedly calling
> nbd_aio_pread from nbdsh in the wrong state.  True, that isn't
> something a sane client would normally do, but it's worth fixing.
> 
> The culprit: when nbd_aio_pread returns a cookie, we guarantee to
> clean up the closure, but if we fail early and never scheduled the
> command, nothing ever cleans up the closure.  We could document this
> as intentional, and force the user to always clean up their closure on
> failure.  However, this is not nice, for two reasons.  First, a
> documentation change to codify existing practice would put more burden
> on clients - every client that calls a C API with closures is now on
> the hook to add boilerplate to avoid a memory leak; and the fact that
> we can easily demonstrate the memory leak in nbdsh means this solution
> does not scale.  Second, consider nbd_pread_structured: that call
> takes a chunk closure parameter, and can fail for multiple reasons,
> either because it was called in the wrong state (in which case we were
> not freeing the closure) or because it succeeded at getting a server
> reponse where the server failed (in which case the closure is freed by
> virtue of getting the server response).  As the caller can't
> distinguish which, telling them to free on failure could result in
> double-frees.
> 
> So the best plan of attack is to document that ALL functions that take
> a closure argument promise to eventually free the closure, even if the
> API fails, and to declare our memory leak as a bug to be fixed in
> libnbd proper.  To do that, the generator will now blindly free
> anything that has not been cleared by the unlocked_* helper, which in
> turn are now careful to clear any callback once it has successfully
> been copied into somewhere that will guarantee subsequent cleanup
> (whether as part of the state machine, or by the fact that
> nbd_internal_command_common now cleans up on all error paths).
> 
> Yes, our change means that any client that WAS checking for nbd_aio_*
> returning -1 for cookies and manually cleaning up is now performing a
> double free, but as argued above, such clients are unlikely because of
> the extra boilerplate it would entail, and because of the fact that
> most clients are sane enough to not trigger error paths that can fail
> client-side without queueing up a trip to the server (for example, no
> one intentionally writes a client that calls nbd_aio_pread before
> nbd_connect_*, except as part of our testsuite in errors.c), to have
> noticed the problem under valgrind before now.
All looks sensible, ACK.

Thanks,

Rich.
>  docs/libnbd.pod           |  2 +-
>  generator/C.ml            | 13 ++++++++
>  lib/debug.c               |  1 +
>  lib/opt.c                 |  5 ++++
>  lib/rw.c                  | 33 ++++++++++++++++----
>  tests/closure-lifetimes.c | 63 ++++++++++++++++++++++++++++++++++++++-
>  tests/newstyle-limited.c  | 18 ++++++++++-
>  7 files changed, 127 insertions(+), 8 deletions(-)
> 
> diff --git a/docs/libnbd.pod b/docs/libnbd.pod
> index 2c3742c..f2ba3bb 100644
> --- a/docs/libnbd.pod
> +++ b/docs/libnbd.pod
> @@ -773,7 +773,7 @@ because you can use closures to achieve the same
effect.
> 
>  You can associate an optional free function with callbacks.  Libnbd
>  will call this function when the callback will not be called again by
> -libnbd.
> +libnbd, including in the case where the API fails.
> 
>  This can be used to free associated C<user_data>.  For example:
> 
> diff --git a/generator/C.ml b/generator/C.ml
> index 280b319..4bdcf60 100644
> --- a/generator/C.ml
> +++ b/generator/C.ml
> @@ -553,6 +553,19 @@ let generate_lib_api_c () >       
print_trace_leave ret;
>        pr "\n"
>      );
> +    (* Finish any closures not transferred to state machine. *)
> +    List.iter (
> +      function
> +      | Closure { cbname } ->
> +         pr "  FREE_CALLBACK (%s_callback);\n" cbname
> +      | _ -> ()
> +    ) args;
> +    List.iter (
> +      function
> +      | OClosure { cbname } ->
> +         pr "  FREE_CALLBACK (%s_callback);\n" cbname
> +      | OFlags _ -> ()
> +    ) optargs;
>      if is_locked then (
>        pr "  if (h->public_state != get_next_state (h))\n";
>        pr "    h->public_state = get_next_state (h);\n";
> diff --git a/lib/debug.c b/lib/debug.c
> index 1b503d9..b598ad3 100644
> --- a/lib/debug.c
> +++ b/lib/debug.c
> @@ -54,6 +54,7 @@ nbd_unlocked_set_debug_callback (struct nbd_handle *h,
>    nbd_unlocked_clear_debug_callback (h);
> 
>    h->debug_callback = *debug_callback;
> +  SET_CALLBACK_TO_NULL (*debug_callback);
>    return 0;
>  }
> 
> diff --git a/lib/opt.c b/lib/opt.c
> index 003ecf8..6ea8326 100644
> --- a/lib/opt.c
> +++ b/lib/opt.c
> @@ -156,6 +156,7 @@ nbd_unlocked_opt_list (struct nbd_handle *h,
nbd_list_callback *list)
>    if (nbd_unlocked_aio_opt_list (h, &l, &c) == -1)
>      return -1;
> 
> +  SET_CALLBACK_TO_NULL (*list);
>    if (wait_for_option (h) == -1)
>      return -1;
>    if (s.err) {
> @@ -172,6 +173,7 @@ nbd_unlocked_aio_opt_go (struct nbd_handle *h,
>  {
>    h->opt_current = NBD_OPT_GO;
>    h->opt_cb.completion = *complete;
> +  SET_CALLBACK_TO_NULL (*complete);
> 
>    if (nbd_internal_run (h, cmd_issue) == -1)
>      debug (h, "option queued, ignoring state machine failure");
> @@ -190,6 +192,7 @@ nbd_unlocked_aio_opt_info (struct nbd_handle *h,
> 
>    h->opt_current = NBD_OPT_INFO;
>    h->opt_cb.completion = *complete;
> +  SET_CALLBACK_TO_NULL (*complete);
> 
>    if (nbd_internal_run (h, cmd_issue) == -1)
>      debug (h, "option queued, ignoring state machine failure");
> @@ -219,7 +222,9 @@ nbd_unlocked_aio_opt_list (struct nbd_handle *h,
nbd_list_callback *list,
> 
>    assert (CALLBACK_IS_NULL (h->opt_cb.fn.list));
>    h->opt_cb.fn.list = *list;
> +  SET_CALLBACK_TO_NULL (*list);
>    h->opt_cb.completion = *complete;
> +  SET_CALLBACK_TO_NULL (*complete);
>    h->opt_current = NBD_OPT_LIST;
>    if (nbd_internal_run (h, cmd_issue) == -1)
>      debug (h, "option queued, ignoring state machine failure");
> diff --git a/lib/rw.c b/lib/rw.c
> index 9f2909b..4b8eeaf 100644
> --- a/lib/rw.c
> +++ b/lib/rw.c
> @@ -72,6 +72,7 @@ nbd_unlocked_pread_structured (struct nbd_handle *h, void
*buf,
>    if (cookie == -1)
>      return -1;
> 
> +  assert (CALLBACK_IS_NULL (*chunk));
>    return wait_for_command (h, cookie);
>  }
> 
> @@ -163,6 +164,7 @@ nbd_unlocked_block_status (struct nbd_handle *h,
>    if (cookie == -1)
>      return -1;
> 
> +  assert (CALLBACK_IS_NULL (*extent));
>    return wait_for_command (h, cookie);
>  }
> 
> @@ -176,11 +178,11 @@ nbd_internal_command_common (struct nbd_handle *h,
> 
>    if (h->disconnect_request) {
>        set_error (EINVAL, "cannot request more commands after
NBD_CMD_DISC");
> -      return -1;
> +      goto err;;
>    }
>    if (h->in_flight == INT_MAX) {
>        set_error (ENOMEM, "too many commands already in flight");
> -      return -1;
> +      goto err;
>    }
> 
>    switch (type) {
> @@ -190,7 +192,7 @@ nbd_internal_command_common (struct nbd_handle *h,
>      if (count > MAX_REQUEST_SIZE) {
>        set_error (ERANGE, "request too large: maximum request size is
%d",
>                   MAX_REQUEST_SIZE);
> -      return -1;
> +      goto err;
>      }
>      break;
> 
> @@ -203,7 +205,7 @@ nbd_internal_command_common (struct nbd_handle *h,
>      if (count > UINT32_MAX) {
>        set_error (ERANGE, "request too large: maximum request size is
%" PRIu32,
>                   UINT32_MAX);
> -      return -1;
> +      goto err;
>      }
>      break;
>    }
> @@ -211,7 +213,7 @@ nbd_internal_command_common (struct nbd_handle *h,
>    cmd = calloc (1, sizeof *cmd);
>    if (cmd == NULL) {
>      set_error (errno, "calloc");
> -    return -1;
> +    goto err;
>    }
>    cmd->flags = flags;
>    cmd->type = type;
> @@ -257,6 +259,17 @@ nbd_internal_command_common (struct nbd_handle *h,
>    }
> 
>    return cmd->cookie;
> +
> + err:
> +  /* Since we did not queue the command, we must free the callbacks. */
> +  if (cb) {
> +    if (type == NBD_CMD_BLOCK_STATUS)
> +      FREE_CALLBACK (cb->fn.extent);
> +    if (type == NBD_CMD_READ)
> +      FREE_CALLBACK (cb->fn.chunk);
> +    FREE_CALLBACK (cb->completion);
> +  }
> +  return -1;
>  }
> 
>  int64_t
> @@ -276,6 +289,7 @@ nbd_unlocked_aio_pread (struct nbd_handle *h, void
*buf,
>      return -1;
>    }
> 
> +  SET_CALLBACK_TO_NULL (*completion);
>    return nbd_internal_command_common (h, 0, NBD_CMD_READ, offset, count,
>                                        buf, &cb);
>  }
> @@ -301,6 +315,8 @@ nbd_unlocked_aio_pread_structured (struct nbd_handle
*h, void *buf,
>      return -1;
>    }
> 
> +  SET_CALLBACK_TO_NULL (*chunk);
> +  SET_CALLBACK_TO_NULL (*completion);
>    return nbd_internal_command_common (h, flags, NBD_CMD_READ, offset,
count,
>                                        buf, &cb);
>  }
> @@ -329,6 +345,7 @@ nbd_unlocked_aio_pwrite (struct nbd_handle *h, const
void *buf,
>      return -1;
>    }
> 
> +  SET_CALLBACK_TO_NULL (*completion);
>    return nbd_internal_command_common (h, flags, NBD_CMD_WRITE, offset,
count,
>                                        (void *) buf, &cb);
>  }
> @@ -350,6 +367,7 @@ nbd_unlocked_aio_flush (struct nbd_handle *h,
>      return -1;
>    }
> 
> +  SET_CALLBACK_TO_NULL (*completion);
>    return nbd_internal_command_common (h, 0, NBD_CMD_FLUSH, 0, 0,
>                                        NULL, &cb);
>  }
> @@ -387,6 +405,7 @@ nbd_unlocked_aio_trim (struct nbd_handle *h,
>      return -1;
>    }
> 
> +  SET_CALLBACK_TO_NULL (*completion);
>    return nbd_internal_command_common (h, flags, NBD_CMD_TRIM, offset,
count,
>                                        NULL, &cb);
>  }
> @@ -413,6 +432,7 @@ nbd_unlocked_aio_cache (struct nbd_handle *h,
>      return -1;
>    }
> 
> +  SET_CALLBACK_TO_NULL (*completion);
>    return nbd_internal_command_common (h, 0, NBD_CMD_CACHE, offset, count,
>                                        NULL, &cb);
>  }
> @@ -457,6 +477,7 @@ nbd_unlocked_aio_zero (struct nbd_handle *h,
>      return -1;
>    }
> 
> +  SET_CALLBACK_TO_NULL (*completion);
>    return nbd_internal_command_common (h, flags, NBD_CMD_WRITE_ZEROES,
offset,
>                                        count, NULL, &cb);
>  }
> @@ -493,6 +514,8 @@ nbd_unlocked_aio_block_status (struct nbd_handle *h,
>      return -1;
>    }
> 
> +  SET_CALLBACK_TO_NULL (*extent);
> +  SET_CALLBACK_TO_NULL (*completion);
>    return nbd_internal_command_common (h, flags, NBD_CMD_BLOCK_STATUS,
offset,
>                                        count, NULL, &cb);
>  }
> diff --git a/tests/closure-lifetimes.c b/tests/closure-lifetimes.c
> index 70788b6..a7a1e46 100644
> --- a/tests/closure-lifetimes.c
> +++ b/tests/closure-lifetimes.c
> @@ -1,5 +1,5 @@
>  /* NBD client library in userspace
> - * Copyright (C) 2013-2019 Red Hat Inc.
> + * Copyright (C) 2013-2020 Red Hat Inc.
>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Lesser General Public
> @@ -42,6 +42,8 @@ static unsigned debug_fn_called;
>  static unsigned debug_fn_freed;
>  static unsigned read_cb_called;
>  static unsigned read_cb_freed;
> +static unsigned block_status_cb_called;
> +static unsigned block_status_cb_freed;
>  static unsigned completion_cb_called;
>  static unsigned completion_cb_freed;
> 
> @@ -74,6 +76,21 @@ read_cb_free (void *opaque)
>    read_cb_freed++;
>  }
> 
> +static int
> +block_status_cb (void *opaque, const char *meta, uint64_t offset,
> +                 uint32_t *entries, size_t nr_entries, int *error)
> +{
> +  assert (!block_status_cb_freed);
> +  block_status_cb_called++;
> +  return 0;
> +}
> +
> +static void
> +block_status_cb_free (void *opaque)
> +{
> +  read_cb_freed++;
> +}
> +
>  static int
>  completion_cb (void *opaque, int *error)
>  {
> @@ -168,5 +185,49 @@ main (int argc, char *argv[])
>    assert (read_cb_freed == 1);
>    assert (completion_cb_freed == 1);
> 
> +  /* Test command callbacks are freed if the command fails client-side,
> +   * whether from calling in wrong state or because of no server support.
> +   */
> +  block_status_cb_called = block_status_cb_freed > +   
completion_cb_called = completion_cb_freed = 0;
> +  nbd = nbd_create ();
> +  if (nbd == NULL) NBD_ERROR;
> +  /* Intentionally omit a call to:
> +   *  nbd_add_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION);
> +   */
> +  cookie = nbd_aio_block_status (nbd, sizeof buf, 0,
> +                                 (nbd_extent_callback) { .callback =
block_status_cb,
> +                                                         .free =
block_status_cb_free },
> +                                 (nbd_completion_callback) { .callback =
completion_cb,
> +                                                             .free =
completion_cb_free },
> +                                 0);
> +  if (cookie != -1) {
> +    fprintf (stderr, "%s: Expecting block_status failure\n",
argv[0]);
> +    exit (EXIT_FAILURE);
> +  }
> +  assert (block_status_cb_freed == 1);
> +  assert (completion_cb_freed == 1);
> +
> +  block_status_cb_called = block_status_cb_freed > +   
completion_cb_called = completion_cb_freed = 0;
> +
> +  if (nbd_connect_command (nbd, nbdkit) == -1) NBD_ERROR;
> +
> +  cookie = nbd_aio_block_status (nbd, sizeof buf, 0,
> +                                 (nbd_extent_callback) { .callback =
block_status_cb,
> +                                                         .free =
block_status_cb_free },
> +                                 (nbd_completion_callback) { .callback =
completion_cb,
> +                                                             .free =
completion_cb_free },
> +                                 0);
> +  if (cookie != -1) {
> +    fprintf (stderr, "%s: Expecting block_status failure\n",
argv[0]);
> +    exit (EXIT_FAILURE);
> +  }
> +  assert (block_status_cb_freed == 1);
> +  assert (completion_cb_freed == 1);
> +
> +  nbd_kill_subprocess (nbd, 0);
> +  nbd_close (nbd);
> +
>    exit (EXIT_SUCCESS);
>  }
> diff --git a/tests/newstyle-limited.c b/tests/newstyle-limited.c
> index fd89620..4479a14 100644
> --- a/tests/newstyle-limited.c
> +++ b/tests/newstyle-limited.c
> @@ -84,6 +84,17 @@ list_cb (void *opaque, const char *name, const char
*description)
>    exit (EXIT_FAILURE);
>  }
> 
> +static bool list_freed = false;
> +static void
> +free_list_cb (void *opaque)
> +{
> +  if (list_freed) {
> +    fprintf (stderr, "%s: list callback mistakenly freed twice",
progname);
> +    exit (EXIT_FAILURE);
> +  }
> +  list_freed = true;
> +}
> +
>  int
>  main (int argc, char *argv[])
>  {
> @@ -144,10 +155,15 @@ main (int argc, char *argv[])
>      fprintf (stderr, "unexpected state after negotiating\n");
>      exit (EXIT_FAILURE);
>    }
> -  if (nbd_opt_list (nbd, (nbd_list_callback) { .callback = list_cb }) !=
-1) {
> +  if (nbd_opt_list (nbd, (nbd_list_callback) { .callback = list_cb,
> +                                               .free = free_list_cb }) !=
-1) {
>      fprintf (stderr, "nbd_opt_list: expected failure\n");
>      exit (EXIT_FAILURE);
>    }
> +  if (!list_freed) {
> +    fprintf (stderr, "nbd_opt_list: list closure memory
leak\n");
> +    exit (EXIT_FAILURE);
> +  }
>    if (nbd_get_errno () != ENOTSUP) {
>      fprintf (stderr, "%s: wrong errno value after failed
opt_list\n", argv[0]);
>      exit (EXIT_FAILURE);
> -- 
> 2.28.0
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.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
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org