Bug 101481 - Memory leak on error reply path in _dbus_connection_block_pending_call()
Summary: Memory leak on error reply path in _dbus_connection_block_pending_call()
Status: VERIFIED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: Other All
: medium minor
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review+
Keywords: patch
Depends on:
Blocks:
 
Reported: 2017-06-17 12:22 UTC by shin1morita
Modified: 2017-07-01 06:32 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Fix dbus_message_unref() is not called (5.01 KB, patch)
2017-06-17 12:22 UTC, shin1morita
Details | Splinter Review
Fix unref NULL (901 bytes, patch)
2017-06-19 22:31 UTC, shin1morita
Details | Splinter Review
Fix exit (0) even if unsuccessful (1.04 KB, patch)
2017-06-19 22:31 UTC, shin1morita
Details | Splinter Review
test-pending-call-dispatch: Add copyright and license grant (2.43 KB, patch)
2017-06-20 09:09 UTC, Simon McVittie
Details | Splinter Review
Squashed: Fix dbus_message_unref() is not called (6.27 KB, patch)
2017-06-20 13:04 UTC, shin1morita
Details | Splinter Review
Revised: Fix dbus_message_unref() is not called (6.28 KB, patch)
2017-06-21 12:33 UTC, shin1morita
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description shin1morita 2017-06-17 12:22:55 UTC
Created attachment 132021 [details] [review]
Fix dbus_message_unref() is not called

dbus_message_unref() is not called for messages containing DBUS_ERROR_DISCONNECTED when dbus_pending_call_block() is called after the connection is disconnected.
Comment 1 Philip Withnall 2017-06-19 12:26:35 UTC
Comment on attachment 132021 [details] [review]
Fix dbus_message_unref() is not called

Review of attachment 132021 [details] [review]:
-----------------------------------------------------------------

This looks good, thanks. One minor comment.

::: test/name-test/test-pending-call-disconnected.c
@@ +1,4 @@
> +/**
> +* Test to make sure that pending calls unref error messages
> +* when blocked after disconnected.
> +**/

It would be good to add a license header here (the fact that some of the other unit tests omit them is a bug).

@smcv, which is the preferred license header formatting to use at the moment?
Comment 2 Simon McVittie 2017-06-19 18:58:00 UTC
(In reply to Philip Withnall from comment #1)
> @smcv, which is the preferred license header formatting to use at the moment?

(I have not reviewed the patch in any detail)

For code derived only from files with a MIT/X11 license grant (like test/dbus-daemon.c) or for new test code from scratch, the MIT/X11 license grant as used in test/dbus-daemon.c is preferred.

For code that is a derivative work of the rest of dbus please use the same GPL-2+|AFL-2.0 license grant as dbus/dbus-connection.c.

I'd prefer new tests to look more like the ones that use GLib (test/dbus-daemon-eavesdrop.c is a relatively simple example) and not be in test/name-test/, but tests in an older style in name-test/ are better than no tests.
Comment 3 Simon McVittie 2017-06-19 19:02:44 UTC
Comment on attachment 132021 [details] [review]
Fix dbus_message_unref() is not called

Review of attachment 132021 [details] [review]:
-----------------------------------------------------------------

::: dbus/dbus-connection.c
@@ +2476,4 @@
>  
>        /* on OOM error_msg is set to NULL */
>        complete_pending_call_and_unlock (connection, pending, error_msg);
> +      dbus_message_unref (error_msg);

This needs to be guarded by

if (error_msg != NULL)

because in the coding conventions used by dbus, it is wrong to unref NULL. The comment 2 lines earlier documents that error_msg might be NULL, and if you look at the implementation of generate_local_error_message() you'll see that it's true.

::: test/name-test/test-pending-call-disconnected.c
@@ +82,5 @@
> +
> +  printf (count == 0 ? "ok\n" : "not ok # Not all refs were unrefed ***\n");
> +
> +  printf ("# Testing completed\n1..1\n");
> +  exit (0);

Please exit (1) if unsuccessful (which in this case means count != 0).
Comment 4 shin1morita 2017-06-19 22:31:08 UTC
Created attachment 132071 [details] [review]
Fix unref NULL
Comment 5 shin1morita 2017-06-19 22:31:51 UTC
Created attachment 132072 [details] [review]
Fix exit (0) even if unsuccessful
Comment 6 shin1morita 2017-06-19 22:50:49 UTC
Philip and Simon, thank you for the comments.

(In reply to Simon McVittie from comment #2)
> For code derived only from files with a MIT/X11 license grant (like
> test/dbus-daemon.c) or for new test code from scratch, the MIT/X11 license
> grant as used in test/dbus-daemon.c is preferred.

It's derived from test-pendinc-call-dispatch.c which is not clear about license grant.
Maybe MIT/X11 is the right choice?

> I'd prefer new tests to look more like the ones that use GLib
> (test/dbus-daemon-eavesdrop.c is a relatively simple example) and not be in
> test/name-test/, but tests in an older style in name-test/ are better than
> no tests.

Oh I'm sorry but I was not aware of that.
I decided where to write it reading the line 287 of HACKING.
Comment 7 shin1morita 2017-06-19 22:52:49 UTC
(In reply to Simon McVittie from comment #3)
> This needs to be guarded by
> if (error_msg != NULL)
> 
> Please exit (1) if unsuccessful (which in this case means count != 0).

Thank you for the comments.
I attached additional patches.
Comment 8 Simon McVittie 2017-06-20 08:47:59 UTC
(In reply to shin1morita from comment #6)
> It's derived from test-pendinc-call-dispatch.c which is not clear about
> license grant.

Judging by its git history, it was written in 2006 by John Palmieri (Red Hat), and since then it has only received trivial patches (which are probably not copyrightable) from Kjartan Maraas, Marcus Brinkmann, David Zeuthen (Red Hat), Simon McVittie (Collabora), Philip Withnall (Endless).

Red Hat have agreed in the past that their dbus contributions could be relicensed under the MIT/X11 license, so if you are OK with doing the same, I think we can use the same MIT/X11 license grant as test/dbus-daemon.c, with:

Copyright © 2006 Red Hat
Copyright © 2017 (your name)

I'll add a license grant to test-pending-call-dispatch.c while we're looking at that file anyway.
Comment 9 Simon McVittie 2017-06-20 09:09:42 UTC
Created attachment 132080 [details] [review]
test-pending-call-dispatch: Add copyright and license grant

According to git history, this test was written in 2006 by Red Hat
employee John Palmieri and has received only trivial changes since
then. Red Hat gave permission in 2007 for their contributions to
be relicensed under the MIT/X11 license. We cannot take advantage
of that permission to relicense the core library or the dbus-daemon
from GPL-2+|AFL-2.0 to MIT/X11, because one early copyright holder
(CodeFactory AB) could not be traced, but we might as well use a
permissive license for simple test code that has not had
CodeFactory AB contributions.
Comment 10 Philip Withnall 2017-06-20 12:30:31 UTC
(In reply to shin1morita from comment #7)
> (In reply to Simon McVittie from comment #3)
> > This needs to be guarded by
> > if (error_msg != NULL)
> > 
> > Please exit (1) if unsuccessful (which in this case means count != 0).
> 
> Thank you for the comments.
> I attached additional patches.

Please squash the additional patches into the original one, upload a new version of that patch, and mark all the old ones as obsolete in Bugzilla.
Comment 11 Philip Withnall 2017-06-20 12:31:35 UTC
Comment on attachment 132080 [details] [review]
test-pending-call-dispatch: Add copyright and license grant

Review of attachment 132080 [details] [review]:
-----------------------------------------------------------------

++

::: test/name-test/test-pending-call-dispatch.c
@@ +1,2 @@
> +/*
> + * Copyright © 2006 Red Hat Inc.

I assume this UTF-8 corruption is Bugzilla, rather than the patch itself.
Comment 12 shin1morita 2017-06-20 13:04:00 UTC
Created attachment 132085 [details] [review]
Squashed: Fix dbus_message_unref() is not called

Squashed first three patches, copyright and license grant as well.
Comment 13 shin1morita 2017-06-20 13:11:19 UTC
(In reply to Simon McVittie from comment #8)
> Red Hat have agreed in the past that their dbus contributions could be
> relicensed under the MIT/X11 license, so if you are OK with doing the same,
> I think we can use the same MIT/X11 license grant as test/dbus-daemon.c,
> with:
> 
> Copyright © 2006 Red Hat
> Copyright © 2017 (your name)

Of course, I'm OK with it.
I attached the new patch which contains the MIT/X11 license grant and copyright in test-pending-call-disconnected.c.

Thank you.
Comment 14 shin1morita 2017-06-20 13:21:04 UTC
(In reply to Philip Withnall from comment #10)
> Please squash the additional patches into the original one, upload a new
> version of that patch, and mark all the old ones as obsolete in Bugzilla.

I squashed them and attached the new patch.
Please check it out.

Thank you.
Comment 15 Philip Withnall 2017-06-21 10:44:39 UTC
Comment on attachment 132085 [details] [review]
Squashed: Fix dbus_message_unref() is not called

Review of attachment 132085 [details] [review]:
-----------------------------------------------------------------

This looks good to commit with or without the change below, depending on whether smcv applies the patch before you update it (or if he finds other review issues).

::: test/name-test/test-pending-call-disconnected.c
@@ +111,5 @@
> +    }
> +  printf ("ok\n");
> +
> +  printf ("# Testing completed\n1..1\n");
> +  exit (0);

Nitpick: the code would be slightly cleaner if you combined the final two printf() calls (in the success case) and moved them into an `else` block:

if (count != 0)
  {
    …
  }
else
  {
    printf ("ok\n#Testing completed\n1..1\n");
    exit (0);
  }
}
Comment 16 shin1morita 2017-06-21 12:27:08 UTC
(In reply to Philip Withnall from comment #15)
> This looks good to commit with or without the change below, depending on
> whether smcv applies the patch before you update it (or if he finds other
> review issues).
> 
> Nitpick: the code would be slightly cleaner if you combined the final two
> printf() calls (in the success case) and moved them into an `else` block:
> 
> else
>   {
>     printf ("ok\n#Testing completed\n1..1\n");
>     exit (0);
>   }

Oh, thank you. I agree with you.
I would replace the patch with new one.
Comment 17 shin1morita 2017-06-21 12:33:27 UTC
Created attachment 132115 [details] [review]
Revised: Fix dbus_message_unref() is not called

Merged Philip's suggestion.
Comment 18 Simon McVittie 2017-06-28 18:08:05 UTC
Thanks, fixed in git for 1.10.20 and 1.11.14.
Comment 19 shin1morita 2017-07-01 06:32:01 UTC
Thank you.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.