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