|
Description
Simon McVittie
2018-07-11 17:33:16 UTC
Created attachment 140563 [details] [review] [1] Assert that DBUS_SERVER_LISTEN_ADDRESS_ALREADY_USED does not set error The only place this is set in practice is in dbus-server-win.c, which does not set the error. If it did, dbus_server_listen() would leak it. Created attachment 140564 [details] [review] [2] dbus_server_listen: Assert that implementations return a known result If they didn't, we'd probably leak the server and/or the error. --- Ideally this should probably be a switch() over an enum, but that would probably mean reindenting and a lot more diffstat. Created attachment 140565 [details] [review] [3] loopback: Don't free credentials borrowed from the connection We currently get away with this because the connection isn't fully freed before we exit, but the connection is meant to own the result of _dbus_connection_get_credentials() (it's "(transfer none)" in GLib terminology). --- An actual bug, albeit in a test. Created attachment 140566 [details] [review] [4] dbus_server_listen: Don't leak first_connect_error If an implementation fails to listen, and a subsequent implementation succeeds, then we would have leaked this. Detected by running tests/loopback.c under valgrind. --- I don't think this is particularly serious, because listening O(n) times is rare, although it could be significant for Containers. Created attachment 140567 [details] [review] [5] tests: Detach server from main loop during teardown test_server_setup() takes a reference to the DBusServer, so we need to release that ref by calling test_server_shutdown(). test_server_shutdown() also disconnects the server, so we don't need to do that. Created attachment 140568 [details] [review] [6] tests: Interpret empty command-line arguments as --tap AX_VALGRIND_CHECK overrides LOG_COMPILER, which means we can't rely on running under glib-tap-test.sh. Default to TAP mode by modifying our (effective) argv instead. If you really want the default behaviour (unstructured output) this can still be achieved by adding some arguments that are a no-op, such as `-m quick`. --- I'm not particularly happy about this, but it's the best I can do right now. I don't really want to rely on a newer GLib or a newer AX_VALGRIND_CHECK because that would make it harder to test on older OSs. TODO: I should probably enhance g_test_init() so it can take an "always use TAP" option after the argv, even if dbus isn't going to make use of that functionality yet. Created attachment 140569 [details] [review] [7] tests: Call dbus_shutdown() Not all of these tests will be fully valgrind-clean yet (or perhaps ever), but it's easier to add this to all of them than to think about it. --- test/name-test/ excluded because adding dbus_shutdown() in obvious places in some of those made them fail, and I really didn't feel like debugging them right now; test/name-test/ needs to disappear one day, and because it's very reliant on the LOG_COMPILER we set, I'll have to exclude it from AX_VALGRIND_CHECK in the short to medium term in any case. Comment on attachment 140563 [details] [review] [1] Assert that DBUS_SERVER_LISTEN_ADDRESS_ALREADY_USED does not set error Review of attachment 140563 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 140564 [details] [review] [2] dbus_server_listen: Assert that implementations return a known result Review of attachment 140564 [details] [review]: ----------------------------------------------------------------- r+, though I’d also suggest changing the if…else if…else to a switch, so that -Wswitch-enum can help us here. That can be a follow-up patch, if you think it’s worthwhile. Comment on attachment 140565 [details] [review] [3] loopback: Don't free credentials borrowed from the connection Review of attachment 140565 [details] [review]: ----------------------------------------------------------------- r+, although while reviewing this I noticed that _dbus_transport_get_credentials() returns FALSE instead of NULL in an early return case. I guess that’s another patch to do. Created attachment 140570 [details] [review] WIP: Use AX_VALGRIND_CHECK to set up partial valgrind test coverage This deliberately doesn't cover name-test/ yet. --- This is incomplete: I probably need to fix a lot more leaks before it will pass. Comment on attachment 140570 [details] [review] WIP: Use AX_VALGRIND_CHECK to set up partial valgrind test coverage Review of attachment 140570 [details] [review]: ----------------------------------------------------------------- ::: configure.ac @@ +1201,5 @@ > > +# Add check-valgrind, etc. targets > +AX_VALGRIND_CHECK > +AM_EXTRA_RECURSIVE_TARGETS([check-valgrind]) > +AM_EXTRA_RECURSIVE_TARGETS([check-valgrind-memcheck]) I don't think we're meant to need to do this, because AX_VALGRIND_CHECK is meant to do it already... but just using AX_VALGRIND_CHECK doesn't seem to add the recursive targets. Do you happen to have an example of a project where AX_VALGRIND_CHECK works? Created attachment 140571 [details] [review] WIP: Make loopback test valgrind-clean --- Probably not all of this is needed, and the bits that are needed should probably be done in all the tests where they're applicable, like I did for calling dbus_shutdown() in Attachment #140569 [details] and for test_server_shutdown() in Attachment #140567 [details]. Comment on attachment 140566 [details] [review] [4] dbus_server_listen: Don't leak first_connect_error Review of attachment 140566 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 140567 [details] [review] [5] tests: Detach server from main loop during teardown Review of attachment 140567 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 140568 [details] [review] [6] tests: Interpret empty command-line arguments as --tap Review of attachment 140568 [details] [review]: ----------------------------------------------------------------- r+, though it’s somewhat hacky Comment on attachment 140569 [details] [review] [7] tests: Call dbus_shutdown() Review of attachment 140569 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 140570 [details] [review] WIP: Use AX_VALGRIND_CHECK to set up partial valgrind test coverage Review of attachment 140570 [details] [review]: ----------------------------------------------------------------- r-, see comments ::: configure.ac @@ +1201,5 @@ > > +# Add check-valgrind, etc. targets > +AX_VALGRIND_CHECK > +AM_EXTRA_RECURSIVE_TARGETS([check-valgrind]) > +AM_EXTRA_RECURSIVE_TARGETS([check-valgrind-memcheck]) https://gitlab.com/walbottle/walbottle is an example of a project where AX_VALGRIND_CHECK has worked for me in the past. I think AX_VALGRIND_CHECK doesn’t include AM_EXTRA_RECURSIVE_TARGETS calls because I didn’t know they existed when I wrote the macro — you could submit a pull request to add them to AX_VALGRIND_CHECK (https://github.com/autoconf-archive/autoconf-archive) and include a copy of the modified macro in-tree in dbus.git. In other projects, I’ve always ignored the need for recursive check-valgrind targets, and have just used `make -C path/to/tests check-valgrind` to run it. Note that if you just want to enable memcheck, you should use AX_VALGRIND_DFLT([sgcheck],[off]) etc. to disable the other Valgrind tools. ::: test/dbus.supp @@ +10,5 @@ > + ... > + fun:gobject_init > +} > +{ > + <insert_a_suppression_name_here> Nitpick: You may want to name this suppression. ::: test/name-test/Makefile.am @@ +90,5 @@ > endif > + > +# Don't use VALGRIND_CHECK_RULES here yet: we're very reliant on > +# LOG_COMPILER for the tests in this directory, and can't cope with > +# AX_VALGRIND_CHECK overriding it to be a valgrind tool. Nitpick: Maybe include ‘FIXME’ in this comment? Comment on attachment 140571 [details] [review] WIP: Make loopback test valgrind-clean Review of attachment 140571 [details] [review]: ----------------------------------------------------------------- r-, though it’s marked as WIP anyway ::: test/loopback.c @@ +437,5 @@ > + { > + test_connection_shutdown (f->ctx, f->client_conn); > + dbus_connection_close (f->client_conn); > + > + while ((message = dbus_connection_pop_message (f->server_conn))) f->client_conn? Created attachment 140598 [details] [review] [8] Allow longer for tests under valgrind Created attachment 140599 [details] [review] [9] test/dbus-daemon: Don't leak error if no machine ID was found --- This is a real corner case, and I only found it because valgrind is currently broken on Debian unstable so I'm doing my testing in a Debian 9 chroot with no init, dbus, /etc/machine-id or /var/lib/dbus/machine-id. Created attachment 140600 [details] [review] [10] test/dbus-daemon: Don't leak expected error for max connections Created attachment 140601 [details] [review] [11] nonce: Don't try to rmdir(NULL) on OOM If re-initializing the string fails, it will be left in a state where it has a length of 0 and a NULL buffer. That's valid to "free", but not valid to pass to rmdir(). Created attachment 140602 [details] [review] [12] test/marshal: Don't leak a message and its marshalled buffer Created attachment 140603 [details] [review] [13] test/containers: Fix some memory leaks Created attachment 140604 [details] [review] [14] tests: Detach most connections from main loop before closing We don't need to do this for connections that were never set up with the main loop. Created attachment 140605 [details] [review] [15] Skip name-test/ when running under valgrind for now These tests are very reliant on their custom LOG_COMPILER, which AX_VALGRIND_CHECK replaces. Created attachment 140606 [details] [review] [16] dispatch test: Simplify OOM testing Instead of having separate test wrappers for the cases that do and don't take a DBusConnection, we can just pass a NULL DBusConnection to the one that doesn't. --- The naming is now even less justifiable than when these functions were added 15 years ago, but I'm not renaming Check2 to something more sensible right now to keep the diffstat down. Created attachment 140607 [details] [review] [17] Don't do OOM testing under valgrind by default It's just painfully slow, particularly when we fork (as we do in test-bus to test service activation). --- At some point I'll leave a machine running for however many days it takes to see whether the slow tests are valgrind-clean, but today is not that day. (In reply to Philip Withnall from comment #18) > I think AX_VALGRIND_CHECK > doesn’t include AM_EXTRA_RECURSIVE_TARGETS calls because I didn’t know they > existed when I wrote the macro The version in autoconf-archive does include them (someone added them after you contributed the macro); but they still don't seem to *work* unless I put them into configure.ac directly. > In other projects, I’ve always ignored the need for recursive check-valgrind > targets, and have just used `make -C path/to/tests check-valgrind` to run it. FYI, since their conversion into recursive targets, check-valgrind-am has that role instead. > Note that if you just want to enable memcheck, you should use > AX_VALGRIND_DFLT([sgcheck],[off]) etc. to disable the other Valgrind tools. Done, because I don't want to get into importing the bits of glib.supp that pacify drd right now... > Nitpick: You may want to name this suppression. Done, and added some more > > +# Don't use VALGRIND_CHECK_RULES here yet > > Nitpick: Maybe include ‘FIXME’ in this comment? Superseded by Attachment #140605 [details] which makes these tests be SKIP'd with an explanatory message, which seems an equally good indicator that they should eventually be fixed (by turning them into self-contained tests that fork their own dbus-daemon) Comment on attachment 140598 [details] [review] [8] Allow longer for tests under valgrind Review of attachment 140598 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 140599 [details] [review] [9] test/dbus-daemon: Don't leak error if no machine ID was found Review of attachment 140599 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 140600 [details] [review] [10] test/dbus-daemon: Don't leak expected error for max connections Review of attachment 140600 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 140601 [details] [review] [11] nonce: Don't try to rmdir(NULL) on OOM Review of attachment 140601 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 140602 [details] [review] [12] test/marshal: Don't leak a message and its marshalled buffer Review of attachment 140602 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 140603 [details] [review] [13] test/containers: Fix some memory leaks Review of attachment 140603 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 140604 [details] [review] [14] tests: Detach most connections from main loop before closing Review of attachment 140604 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 140605 [details] [review] [15] Skip name-test/ when running under valgrind for now Review of attachment 140605 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 140606 [details] [review] [16] dispatch test: Simplify OOM testing Review of attachment 140606 [details] [review]: ----------------------------------------------------------------- r+, neat Comment on attachment 140607 [details] [review] [17] Don't do OOM testing under valgrind by default Review of attachment 140607 [details] [review]: ----------------------------------------------------------------- r+, seems reasonable (In reply to Simon McVittie from comment #2) > [2] dbus_server_listen: Assert that implementations return a known result > > Ideally this should probably be a switch() over an enum, but that would > probably mean reindenting and a lot more diffstat. TODO, but not blocking on this (In reply to Philip Withnall from comment #10) > [3] loopback: Don't free credentials borrowed from the connection > > r+, although while reviewing this I noticed that > _dbus_transport_get_credentials() returns FALSE instead of NULL in an early > return case. I guess that’s another patch to do. TODO, but not blocking on this (In reply to Simon McVittie from comment #41) > (In reply to Simon McVittie from comment #2) > > [2] dbus_server_listen: Assert that implementations return a known result > > > > Ideally this should probably be a switch() over an enum, but that would > > probably mean reindenting and a lot more diffstat. > > TODO, but not blocking on this 👍 > (In reply to Philip Withnall from comment #10) > > [3] loopback: Don't free credentials borrowed from the connection > > > > r+, although while reviewing this I noticed that > > _dbus_transport_get_credentials() returns FALSE instead of NULL in an early > > return case. I guess that’s another patch to do. > > TODO, but not blocking on this 👍 Comment on attachment 140563 [details] [review] [1] Assert that DBUS_SERVER_LISTEN_ADDRESS_ALREADY_USED does not set error Applied for 1.13.6 Comment on attachment 140564 [details] [review] [2] dbus_server_listen: Assert that implementations return a known result Applied for 1.13.6 Comment on attachment 140565 [details] [review] [3] loopback: Don't free credentials borrowed from the connection Applied for 1.13.6 (not for other branches because it's test-only) Comment on attachment 140566 [details] [review] [4] dbus_server_listen: Don't leak first_connect_error Applied for 1.13.6 and 1.12.10 Comment on attachment 140567 [details] [review] [5] tests: Detach server from main loop during teardown Applied for 1.13.6 Comment on attachment 140568 [details] [review] [6] tests: Interpret empty command-line arguments as --tap Applied for 1.13.6 Comment on attachment 140569 [details] [review] [7] tests: Call dbus_shutdown() Applied for 1.13.6 Comment on attachment 140598 [details] [review] [8] Allow longer for tests under valgrind Applied for 1.13.6 Comment on attachment 140599 [details] [review] [9] test/dbus-daemon: Don't leak error if no machine ID was found Applied for 1.13.6 Comment on attachment 140600 [details] [review] [10] test/dbus-daemon: Don't leak expected error for max connections Applied for 1.13.6 Comment on attachment 140601 [details] [review] [11] nonce: Don't try to rmdir(NULL) on OOM Applied for 1.13.6 and 1.12.10 Comment on attachment 140602 [details] [review] [12] test/marshal: Don't leak a message and its marshalled buffer Applied for 1.13.6 Comment on attachment 140603 [details] [review] [13] test/containers: Fix some memory leaks Applied for 1.13.6 Comment on attachment 140604 [details] [review] [14] tests: Detach most connections from main loop before closing Applied for 1.13.6 Comment on attachment 140605 [details] [review] [15] Skip name-test/ when running under valgrind for now Applied for 1.13.6 Comment on attachment 140606 [details] [review] [16] dispatch test: Simplify OOM testing Applied for 1.13.6 Comment on attachment 140607 [details] [review] [17] Don't do OOM testing under valgrind by default Applied for 1.13.6 Created attachment 140942 [details] [review] [18] embedded tests: Make it easier to run a single test-case When running tests under "make check" or similar to take advantage of facilities like AM_TESTS_ENVIRONMENT and AX_VALGRIND_CHECK, it's more straightforward to set an environment variable than to pass a command-line option. Created attachment 140943 [details] [review] WIP: Use AX_VALGRIND_CHECK to set up partial valgrind test coverage --- Now with documentation. TODO: It would be nice if there was a way to make spawn_dbus_daemon() run the dbus-daemon under valgrind too, but that probably involves fighting with and/or contributing incompatible changes to AX_VALGRIND_CHECK. Comment on attachment 140942 [details] [review] [18] embedded tests: Make it easier to run a single test-case Review of attachment 140942 [details] [review]: ----------------------------------------------------------------- True, dat. r+ Comment on attachment 140943 [details] [review] WIP: Use AX_VALGRIND_CHECK to set up partial valgrind test coverage Review of attachment 140943 [details] [review]: ----------------------------------------------------------------- ::: test/Makefile.am @@ +659,5 @@ > +# Add rules for valgrind testing, as defined by AX_VALGRIND_CHECK > + > +# Potentially useful additions: > +#VALGRIND_FLAGS = --num-callers=30 --gen-suppressions=all --read-var-info=yes > +#VALGRIND_memcheck_FLAGS = --show-leak-kinds=all --errors-for-leak-kinds=definite,possible --leak-check=full OOI, why have these commented out? Are they only meant to be informative? (In reply to Philip Withnall from comment #64) > OOI, why have these commented out? Are they only meant to be informative? Yes, my intention was that they're an opt-in for "show me more leaks, slower" rather than something that developers use all the time. -- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/218. |
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.