| Message ID | 1459526222-30052-1-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01.04.2016 17:57, Eric Blake wrote: > Commit 7836857 introduced a memory leak due to invalid use of > Error vs. visit_type_end(). If visiting the intermediate > members fails, we clear the error and unconditionally use > visit_end_struct() on the same error object; but if that > cleanup succeeds, we then skip the qapi_free call. It's not really a memleak. Due to skipping those conditional branches after the "out" label, a non-null value will be returned. In order to determine whether the function call failed, the callers of these functions do not use the errp value but the return value. Therefore, they will think the call succeeded when actually it did not. > > Until a later patch adds visit_check_struct(), the only safe > approach is to use two separate error objects. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > block/crypto.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) Anyway, thanks, applied to my block branch: https://github.com/XanClic/qemu/commits/block Max
Max Reitz <mreitz@redhat.com> writes: > On 01.04.2016 17:57, Eric Blake wrote: >> Commit 7836857 introduced a memory leak due to invalid use of >> Error vs. visit_type_end(). If visiting the intermediate >> members fails, we clear the error and unconditionally use >> visit_end_struct() on the same error object; but if that >> cleanup succeeds, we then skip the qapi_free call. > > It's not really a memleak. Due to skipping those conditional branches > after the "out" label, a non-null value will be returned. In order to > determine whether the function call failed, the callers of these > functions do not use the errp value but the return value. Therefore, > they will think the call succeeded when actually it did not. Please amend the commit message accordingly. >> >> Until a later patch adds visit_check_struct(), the only safe >> approach is to use two separate error objects. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> block/crypto.c | 12 ++++++------ >> 1 file changed, 6 insertions(+), 6 deletions(-) > > Anyway, thanks, applied to my block branch: > > https://github.com/XanClic/qemu/commits/block > > Max
On 04/07/2016 09:14 AM, Markus Armbruster wrote: > Max Reitz <mreitz@redhat.com> writes: > >> On 01.04.2016 17:57, Eric Blake wrote: >>> Commit 7836857 introduced a memory leak due to invalid use of >>> Error vs. visit_type_end(). If visiting the intermediate >>> members fails, we clear the error and unconditionally use >>> visit_end_struct() on the same error object; but if that >>> cleanup succeeds, we then skip the qapi_free call. >> >> It's not really a memleak. Due to skipping those conditional branches >> after the "out" label, a non-null value will be returned. In order to >> determine whether the function call failed, the callers of these >> functions do not use the errp value but the return value. Therefore, >> they will think the call succeeded when actually it did not. > > Please amend the commit message accordingly. Too late; already merged as 95c3df5a. [And welcome back - hope you don't mind the backlog...] (Locally it looks like a memory leak; it is only the wider analysis that shows that the caller is not leaking things, but where the bug then shifts to being a potential for the caller to abort if it tries to set an error into the already-set errp)
diff --git a/block/crypto.c b/block/crypto.c index be34985..1903e84 100644 --- a/block/crypto.c +++ b/block/crypto.c @@ -196,6 +196,7 @@ block_crypto_open_opts_init(QCryptoBlockFormat format, OptsVisitor *ov; QCryptoBlockOpenOptions *ret = NULL; Error *local_err = NULL; + Error *end_err = NULL; ret = g_new0(QCryptoBlockOpenOptions, 1); ret->format = format; @@ -218,10 +219,9 @@ block_crypto_open_opts_init(QCryptoBlockFormat format, error_setg(&local_err, "Unsupported block format %d", format); break; } - error_propagate(errp, local_err); - local_err = NULL; - visit_end_struct(opts_get_visitor(ov), &local_err); + visit_end_struct(opts_get_visitor(ov), &end_err); + error_propagate(&local_err, end_err); out: if (local_err) { @@ -242,6 +242,7 @@ block_crypto_create_opts_init(QCryptoBlockFormat format, OptsVisitor *ov; QCryptoBlockCreateOptions *ret = NULL; Error *local_err = NULL; + Error *end_err = NULL; ret = g_new0(QCryptoBlockCreateOptions, 1); ret->format = format; @@ -264,10 +265,9 @@ block_crypto_create_opts_init(QCryptoBlockFormat format, error_setg(&local_err, "Unsupported block format %d", format); break; } - error_propagate(errp, local_err); - local_err = NULL; - visit_end_struct(opts_get_visitor(ov), &local_err); + visit_end_struct(opts_get_visitor(ov), &end_err); + error_propagate(&local_err, end_err); out: if (local_err) {
Commit 7836857 introduced a memory leak due to invalid use of Error vs. visit_type_end(). If visiting the intermediate members fails, we clear the error and unconditionally use visit_end_struct() on the same error object; but if that cleanup succeeds, we then skip the qapi_free call. Until a later patch adds visit_check_struct(), the only safe approach is to use two separate error objects. Signed-off-by: Eric Blake <eblake@redhat.com> --- block/crypto.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)