|
Description
Ralf Habacker
2015-04-13 20:58:09 UTC
Created attachment 115063 [details] [review] CID 54854: Dereference null return value (NULL_RETURNS) Created attachment 115064 [details] [review] CID 54846: Dereference null return value (NULL_RETURNS) Comment on attachment 115063 [details] [review] CID 54854: Dereference null return value (NULL_RETURNS) Review of attachment 115063 [details] [review]: ----------------------------------------------------------------- ::: bus/connection.c @@ +2658,4 @@ > BusConnectionData *d; > > d = BUS_CONNECTION_DATA (connection); > + return d ? d->peak_bus_names : 0; Is there any situation in which a connection within the dbus-daemon can validly have no BusConnectionData? If there isn't, I'd prefer to add a _dbus_assert (d != NULL), which iirc is a suitable hint to Coverity that this can't happen. Looks like you attached the patch for CID 54846 twice? If you called them something like "CID 54846: bus_connection_get_peak_bus_names: avoid possible NULL dereference" it would probably be easier to spot that mistake. Created attachment 115065 [details] [review] CID 54854: Dereference null return value (NULL_RETURNS) upload correct patch (In reply to Simon McVittie from comment #3) > Comment on attachment 115063 [details] [review] [review] > CID 54854: Dereference null return value (NULL_RETURNS) > > Review of attachment 115063 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: bus/connection.c > @@ +2658,4 @@ > > BusConnectionData *d; > > > > d = BUS_CONNECTION_DATA (connection); > > + return d ? d->peak_bus_names : 0; > > Is there any situation in which a connection within the dbus-daemon can > validly have no BusConnectionData? connection data is set by calling dbus_connection_set_data(), so in theory it would be possible. Searching for the term 'DBUS_CONNECTION_DATA' shows in 35 of 37 cases that _dbus_assert() has been used. Exceptions are: dbus_bool_t bus_connection_is_monitor (DBusConnection *connection) { BusConnectionData *d; d = BUS_CONNECTION_DATA (connection); return d != NULL && d->link_in_monitors != NULL; } dbus_bool_t bus_connection_is_active (DBusConnection *connection) { BusConnectionData *d; d = BUS_CONNECTION_DATA (connection); return d != NULL && d->name != NULL; } > If there isn't, I'd prefer to add a _dbus_assert (d != NULL), which iirc is > a suitable hint to Coverity that this can't happen. The functions covered by the patches are not used in the code, so it is hard to find an answer from the context. Created attachment 115068 [details] [review] CID 54766: Resource leak (RESOURCE_LEAK). (In reply to Ralf Habacker from comment #6) > (In reply to Simon McVittie from comment #3) > > Is there any situation in which a connection within the dbus-daemon can > > validly have no BusConnectionData? > > connection data is set by calling dbus_connection_set_data(), so in theory > it would be possible. Right, but does dbus-daemon ever operate on a DBusConnection where we have not called dbus_connection_set_data()? AIUI, the answer is "no, and if it somehow did, nothing would work correctly". My understanding is that dbus-daemon's only source of DBusConnection objects is its DBusServer, which does call that function (among other setup) before doing anything else with the connection. If this was GLib code, I'd be using g_return_val_if_fail(); but dbus has the policy that "checks" (the equivalent of g_return_val_if_fail()) are only allowed in public API functions, and internal functions only use assertions. Maybe we should change that policy... > dbus_bool_t > bus_connection_is_active (DBusConnection *connection) > { > BusConnectionData *d; > > d = BUS_CONNECTION_DATA (connection); > > return d != NULL && d->name != NULL; > } It seems more likely to me that this function could conceivably be called "early". If it isn't (check the places in dbus-daemon that produce a new DBusConnection, i.e. the DBusServer), I'd be OK with rewriting it to have a structure more like this: BCD *d; d = B_C_D (connection); _dbus_assert (d != NULL); return d->name != NULL; bus_connection_is_monitor() is essentially a copy of this function, hence the similar structure. > The functions covered by the patches are not used in the code, so it is hard > to find an answer from the context. Huh? Surely every non-main() function in dbus-daemon is either called by another function in dbus-daemon, or dead code that should be deleted? Comment on attachment 115068 [details] [review] CID 54766: Resource leak (RESOURCE_LEAK). Review of attachment 115068 [details] [review]: ----------------------------------------------------------------- OK for master. Please don't apply to stable-branches, since this is only test code. (One of the problems with starting to use a static analysis tool like Coverity is that they treat all findings as equally important, so there's often a backlog of unimportant fixes in tests and other secondary code.) Comment on attachment 115068 [details] [review] CID 54766: Resource leak (RESOURCE_LEAK). committed to master (In reply to Simon McVittie from comment #8) > (In reply to Ralf Habacker from comment #6) > > (In reply to Simon McVittie from comment #3) > > > Is there any situation in which a connection within the dbus-daemon can > > > validly have no BusConnectionData? > > > > connection data is set by calling dbus_connection_set_data(), so in theory > > it would be possible. > > Right, but does dbus-daemon ever operate on a DBusConnection where we have > not called dbus_connection_set_data()? AIUI, the answer is "no, and if it > somehow did, nothing would work correctly". > My understanding is that dbus-daemon's only source of DBusConnection objects > is its DBusServer, which does call that function (among other setup) before > doing anything else with the connection. A breakpoint at dbus_connection_set_data on dbus-daemon shows [1] 0 dbus_connection_set_data dbus-connection.c 6003 0x7ffff79361d4 1 bus_connections_setup_connection connection.c 700 0x41f961 2 new_connection_callback bus.c 176 0x41413a 3 handle_new_client_fd_and_unlock dbus-server-socket.c 145 0x7ffff795b15b 4 socket_handle_watch dbus-server-socket.c 212 0x7ffff795b3e0 5 dbus_watch_handle dbus-watch.c 722 0x7ffff79679f6 6 _dbus_loop_iterate dbus-mainloop.c 818 0x441061 7 _dbus_loop_run dbus-mainloop.c 882 0x441203 8 main main.c 656 0x43f780 > If this was GLib code, I'd be using g_return_val_if_fail(); but dbus has the > policy that "checks" (the equivalent of g_return_val_if_fail()) are only > allowed in public API functions, and internal functions only use assertions. > Maybe we should change that policy... That would help developers immediatly to see that there are major api using issues. > > > dbus_bool_t > > bus_connection_is_active (DBusConnection *connection) > > { > > BusConnectionData *d; > > > > d = BUS_CONNECTION_DATA (connection); > > > > return d != NULL && d->name != NULL; > > } > > It seems more likely to me that this function could conceivably be called > "early". If it isn't (check the places in dbus-daemon that produce a new > DBusConnection, i.e. the DBusServer), A breakpoint at bus_connection_is_active on dbus-daemon shows: 0 bus_connection_is_active connection.c 1255 0x420815 1 bus_dispatch dispatch.c 343 0x4252e0 2 bus_dispatch_message_filter dispatch.c 509 0x42581d 3 dbus_connection_dispatch dbus-connection.c 4677 0x7ffff7932ce9 4 _dbus_loop_dispatch dbus-mainloop.c 530 0x440a09 5 _dbus_loop_iterate dbus-mainloop.c 856 0x44114d 6 _dbus_loop_run dbus-mainloop.c 882 0x441203 7 main main.c 656 0x43f780 which is called later as [1] > I'd be OK with rewriting it to have a > structure more like this: > > BCD *d; > > d = B_C_D (connection); > _dbus_assert (d != NULL); > return d->name != NULL; > > bus_connection_is_monitor() is essentially a copy of this function, hence > the similar structure. so any B_C_D call result could be watched with _dbus_assert > > The functions covered by the patches are not used in the code, so it is hard > > to find an answer from the context. > > Huh? Surely every non-main() function in dbus-daemon is either called by > another function in dbus-daemon, or dead code that should be deleted? got it, they are called by the stats interface, which wasn't enabled by default builds. Created attachment 115072 [details] [review] Unify return value handling of BUS_CONNECTION_DATA(). (In reply to Simon McVittie from comment #9) > Comment on attachment 115068 [details] [review] [review] > CID 54766: Resource leak (RESOURCE_LEAK). > > Review of attachment 115068 [details] [review] [review]: > ----------------------------------------------------------------- > > OK for master. Please don't apply to stable-branches, since this is only > test code. > > (One of the problems with starting to use a static analysis tool like > Coverity is that they treat all findings as equally important, so there's > often a backlog of unimportant fixes in tests and other secondary code.) Unfortunally valgrind for example do not distincts between unimportant and important memory leak; having much unimportant memory leaks in an app hides the real important memory leaks. BTW: I recently added components (libdbus, bus, tools, test) to dbus coverity scan, see https://scan.coverity.com/projects/4774?tab=overview, which gives a better overview over the defect density. (In reply to Ralf Habacker from comment #13) > (In reply to Simon McVittie from comment #9) > > Comment on attachment 115068 [details] [review] [review] [review] > > CID 54766: Resource leak (RESOURCE_LEAK). > > > > Review of attachment 115068 [details] [review] [review] [review]: > > ----------------------------------------------------------------- > > > > OK for master. Please don't apply to stable-branches, since this is only > > test code. > > > > (One of the problems with starting to use a static analysis tool like > > Coverity is that they treat all findings as equally important, so there's > > often a backlog of unimportant fixes in tests and other secondary code.) On the other side the scan shines on some very hidden issues not visible otherwise. One example is: static dbus_bool_t include_dir(...) ... 2265 dir = _dbus_directory_open (dirname, error); 2266 2. Condition dir == NULL, taking true branch 3. var_compare_op: Comparing dir to null implies that dir might be null. 2267 if (dir == NULL) 2268 { 4. Condition dbus_error_has_name(error, "org.freedesktop.DBus.Error.FileNotFound"), taking false branch 2269 if (dbus_error_has_name (error, DBUS_ERROR_FILE_NOT_FOUND)) 2270 { 2271 dbus_error_free (error); 2272 goto success; 2273 } 2274 } 2275 2276 dbus_error_init (&tmp_error); CID 54744 (#1 of 1): Dereference after null check (FORWARD_NULL)5. var_deref_model: Passing null pointer dir to _dbus_directory_get_next_file, which dereferences it. [show details] 2277 while (_dbus_directory_get_next_file (dir, &filename, &tmp_error)) 2278 { The problem here is that _dbus_directory_open() may return zero for other cases then DBUS_ERROR_FILE_NOT_FOUND, which lead to a crash. In other places where _dbus_directory_open() is called all error codes are catched. Created attachment 115073 [details] [review] CID 54744: Dereference after null check (FORWARD_NULL) (In reply to Ralf Habacker from comment #15) > Created attachment 115073 [details] [review] [review] > CID 54744: Dereference after null check (FORWARD_NULL) Turns out that this issue is usable as ddos attack. If an epxloit would make it possible to let dbus_directory_open fail with an error code different than DBUS_ERROR_FILE_NOT_FOUND, dbus-daemon would crash. On linux it uses opendir; the related doc at http://man7.org/linux/man-pages/man3/opendir.3.html lists some of them. EACCES Permission denied. EBADF fd is not a valid file descriptor opened for reading. EMFILE Too many file descriptors in use by process. ENFILE Too many files are currently open in the system. ENOMEM Insufficient memory to complete the operation. ENOTDIR name is not a directory. (In reply to Ralf Habacker from comment #11) > A breakpoint at dbus_connection_set_data on dbus-daemon shows [1] > > 0 dbus_connection_set_data dbus-connection.c 6003 0x7ffff79361d4 > 1 bus_connections_setup_connection connection.c 700 0x41f961 The important question is not "does bus_connections_setup_connection() call dbus_connection_set_data()?" - it does, and git grep could tell you that. The important question is whether dbus-daemon ever operates on a DBusConnection that did not go through bus_connections_setup_connection(). If it does, then the assertion you're adding is not valid. (I think the answer is, no, it does not ever operate on a DBusConnection that did not go through that code path.) > A breakpoint at bus_connection_is_active on dbus-daemon shows: A breakpoint in gdb only tells you where it is called *in your specific set of testing*; it cannot tell you whether it can be called in other call-stacks that you did not test (e.g. because they only happen in really obscure situations, or on a platform you don't have). Only looking at the source code can tell you the complete set of call sites. (In reply to Ralf Habacker from comment #16) > Turns out that this issue is usable as ddos attack. This is certainly not DDOS, because the first D in DDOS is "distributed". If you suspect you have found a D-Bus security flaw, please do not discuss it in public; please contact dbus-security@lists.freedesktop.org instead. However, in this case I don't think it is an exploitable denial-of-service flaw, just a bug: I don't think there's any way for an unprivileged user to tell "dbus-daemon --system" to open additional directories, so an attacker cannot trigger this bug. Comment on attachment 115072 [details] [review] Unify return value handling of BUS_CONNECTION_DATA(). Review of attachment 115072 [details] [review]: ----------------------------------------------------------------- This is OK for master, after checking that we don't have any other sources of DBusConnections in dbus-daemon. Not for 1.8: it's "executable documentation" (hinting Coverity towards not raising false positives) rather than a fix for an actual bug. Comment on attachment 115073 [details] [review] CID 54744: Dereference after null check (FORWARD_NULL) Review of attachment 115073 [details] [review]: ----------------------------------------------------------------- Looks good for 1.8 and master. As noted on one of the other patches, I'd prefer the first line of the commit message to be something that makes sense to people without access to Coverity, maybe something like this: > Subject: include_dir: skip processing on error (CID 54744) > > We already skipped processing for DBUS_ERROR_FILE_NOT_FOUND; > but if the error was something else, we would pass the NULL > pointer dir to _dbus_directory_get_next_file(), which dereferences it. > Coverity: CID 54744: Dereference after null check (FORWARD_NULL) > > Bug: https://bugs.freedesktop.org/show_bug.cgi?id=90021 (In reply to Simon McVittie from comment #19) > Comment on attachment 115073 [details] [review] [review] > CID 54744: Dereference after null check (FORWARD_NULL) > > Review of attachment 115073 [details] [review] [review]: > ----------------------------------------------------------------- > > Looks good for 1.8 and master. patch do not apply to dbus-1.8. The related source shows that dbus-1.8 is not affected. dir = _dbus_directory_open (dirname, error); if (dir == NULL) goto failed; dbus_error_init (&tmp_error); while (_dbus_directory_get_next_file (dir, &filename, &tmp_error)) The bug has been introduced with commit 57971f69ef610079d16e32de78c2dfaf9a8750a1 http://cgit.freedesktop.org/dbus/dbus/commit/?id=57971f69ef610079d16e32de78c2dfaf9a8750a1 > > As noted on one of the other patches, I'd prefer the first line of the > commit message to be something that makes sense to people without access to > Coverity, maybe something like this: okay, based on the related note I added already additional lines to the commit. Will change that. (In reply to Simon McVittie from comment #19) > > Coverity: CID 54744: Dereference after null check (FORWARD_NULL) Will also add a Coverity: key before any coverity reference. (In reply to Simon McVittie from comment #17) > (In reply to Ralf Habacker from comment #11) > > A breakpoint at dbus_connection_set_data on dbus-daemon shows [1] > > > > 0 dbus_connection_set_data dbus-connection.c 6003 0x7ffff79361d4 > > 1 bus_connections_setup_connection connection.c 700 0x41f961 > > The important question is not "does bus_connections_setup_connection() call > dbus_connection_set_data()?" - it does, and git grep could tell you that. > > The important question is whether dbus-daemon ever operates on a > DBusConnection that did not go through bus_connections_setup_connection(). > If it does, then the assertion you're adding is not valid. > > (I think the answer is, no, it does not ever operate on a DBusConnection > that did not go through that code path.) > new connections are created by _dbus_connection_new_for_transport() called on the server side by handle_new_client_fd_and_unlock (DBusServer *server, int client_fd) 0 handle_new_client_fd_and_unlock dbus-server-socket.c 94 0x7ffff795b24b 1 socket_handle_watch dbus-server-socket.c 212 0x7ffff795b7fc 2 dbus_watch_handle dbus-watch.c 722 0x7ffff7967d7a 3 _dbus_loop_iterate dbus-mainloop.c 818 0x43f711 4 _dbus_loop_run dbus-mainloop.c 882 0x43f8b3 5 main main.c 656 0x43de30 If the server has a "new connection callback" registered, it will be called. The dbus-daemon itself registers new_connection_callback(), which calls bus_connections_setup_connection. On the client side it is called by static DBusConnection* connection_try_from_address_entry (DBusAddressEntry *entry,DBusError *error) _dbus_connection_open_internal dbus_connection_open dbus_connection_open_private I did not found any reference of setting up the connection data slot, which will let any bus_connection_get.. referencing the connection data slot run into the assert. and DBusTransport* _dbus_transport_debug_pipe_new (const char *server_name, DBusError *error) which depends on the server side running of the related pipe. It fetches the DBusServer instance and runs it "new connection callback" is defined /* See if someone wants to handle this new connection, * self-referencing for paranoia */ if (server->new_connection_function) { dbus_server_ref (server); (* server->new_connection_function) (server, connection, server->new_connection_data); dbus_server_unref (server); } It did not found any reference that the debug pipe server setup a "new connection callback". (In reply to Ralf Habacker from comment #22) > On the client side it is called by > > static DBusConnection* connection_try_from_address_entry (DBusAddressEntry Right, but does dbus-daemon ever open connections like this? I hope the answer is "no, except for the debug-pipe transport"... > DBusTransport* _dbus_transport_debug_pipe_new (const char *server_name, > DBusError *error) ... and a production dbus-daemon doesn't have that, because it's #ifdef DBUS_ENABLE_EMBEDDED_TESTS. (In reply to Simon McVittie from comment #23) > (In reply to Ralf Habacker from comment #22) > > On the client side it is called by > > > > static DBusConnection* connection_try_from_address_entry (DBusAddressEntry > > Right, but does dbus-daemon ever open connections like this? > I hope the answer is "no, except for the debug-pipe transport"... as stated in comment #11 it is called by code running on the client side. Here is a complete list of related functions. >connection_try_from_address_entry() called by > _dbus_connection_open_internal() called by > dbus_connection_open () called by dbus_bus_get() _dbus_transport_unix_test() open_shutdown_private_connection() open_destroy_shared_session_bus_connection() dbus-monitor.c:main() dbus-send.c:main() > dbus_connection_open_private() called by internal_bus_get() bus_....test() so I would say no. > > > DBusTransport* _dbus_transport_debug_pipe_new (const char *server_name, > > DBusError *error) > > ... and a production dbus-daemon doesn't have that, because it's #ifdef > DBUS_ENABLE_EMBEDDED_TESTS. And someone calling BUS_CONNECTION_DATA related bus_connection... functions with debug pipe connections will get asserts when setting DBUS_ENABLE_EMBEDDED_TESTS. To avoid this a data connection slot should be added to debug pipe server creating. (In reply to Ralf Habacker from comment #24) > And someone calling BUS_CONNECTION_DATA related bus_connection... functions > with debug pipe connections will get asserts when setting > DBUS_ENABLE_EMBEDDED_TESTS. The whole point of DBUS_ENABLE_EMBEDDED_TESTS is that it checks our assumptions, and if the tests fail, it crashes; so I'd consider assertion failures in that mode to be a bug, certainly, but not a denial-of-service flaw. > To avoid this a data connection slot should be added to debug pipe server > creating. It looks as though the only user of the debug-pipe transport in bus/ is in the tests for dispatch.c, which use TEST_DEBUG_PIPE. If those tests pass, we're fine; if they fail, then as you say, something will need modifying so that they can pass. They all call bus_setup_debug_client(), which might be a reasonable place to add a call to bus_connections_setup_connection() if necessary? (I don't like the debug-pipe transport, and I wish we could get rid of it, but I don't think we can do that without considerable work to add an async version of dbus_connection_open_private() - see commit e9f0378b.) (In reply to Simon McVittie from comment #25) > It looks as though the only user of the debug-pipe transport in bus/ is in > the tests for dispatch.c, which use TEST_DEBUG_PIPE. If those tests pass, > we're fine; if they fail, then as you say, something will need modifying so > that they can pass. I think this is actually going to work fine. The weird thing about that test is that it "talks to itself", so client- and dbus-daemon-side connections share a process: fake client | dbus-daemon code | client-side --- DBusServer --- daemon-side connection | connection (no BCD) | (has BCD) "In real life" the vertical line would be a boundary between processes, but for the purposes of this test it isn't. The client side of the pipe doesn't call bus_connections_setup_connection(), so it doesn't have BusConnectionData; but nothing in the dbus-daemon tests should be mixing up client-side and dbus-daemon-side connections anyway, and in particular, nothing should call dbus-daemon-side functions on a client-side connection. So it's valid for dbus-daemon-side code to assert that all connections on which it is called came out of the DBusServer. Comment on attachment 115073 [details] [review] CID 54744: Dereference after null check (FORWARD_NULL) Applied this to master, with my suggested changes to the commit message. (In reply to Ralf Habacker from comment #20) > The bug has been introduced with commit > 57971f69ef610079d16e32de78c2dfaf9a8750a1 > http://cgit.freedesktop.org/dbus/dbus/commit/ > ?id=57971f69ef610079d16e32de78c2dfaf9a8750a1 Good catch. Nothing to be done here for dbus-1.8, then. Comment on attachment 115072 [details] [review] Unify return value handling of BUS_CONNECTION_DATA(). Review of attachment 115072 [details] [review]: ----------------------------------------------------------------- ::: bus/connection.c @@ +645,1 @@ > int n_pending_unix_fds_old = d->n_pending_unix_fds; bus/connection.c:645:3: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] Created attachment 115122 [details] [review] Always assert that BUS_CONNECTION_DATA() returns non-NULL From: Ralf Habacker <ralf.habacker@freenet.de> Every DBusConnection in the dbus-daemon should have been through bus_connections_setup_connection(), so we can assert that the BusConnectionData has been attached to it. Having this assertion is enough to hint to Coverity that it does not need to worry about whether this pointer might be NULL. In regression tests, we do work with a few fake client-side DBusConnection instances in the same process; but it would be a serious bug if we mixed those up with the ones processed by dbus-daemon's real code, so the assertion is still valid. This patch has been inspired by (and fixes) the following coverity scan issues: CID 54846: Dereference null return value (NULL_RETURNS). CID 54854: Dereference null return value (NULL_RETURNS). Bug: https://bugs.freedesktop.org/show_bug.cgi?id=90021 [smcv: fixed -Wdeclaration-after-statement; more informative commit message] --- I'm happy to merge this to master if you're also happy with it. Not suitable for stable-branches - it silences Coverity warnings, but does not fix an actual real-world bug. (In reply to Simon McVittie from comment #29) > I'm happy to merge this to master if you're also happy with it. I'm happy with it. Comment on attachment 115122 [details] [review] Always assert that BUS_CONNECTION_DATA() returns non-NULL Fixed in git for 1.9.16. Leaving this bug open because there are presumably a bunch more Coverity fixes to come. I've done some triaging in Coverity. A lot of the results are trivial things like not checking for OOM in test code, which, while worth fixing in master, are really not very important. Some of the complaints about use-after-free seem to be Coverity not understanding our reference-counting: it thinks something is last-unref even though we're quite confident that we hold a ref in those contexts. It would be worth auditing the relevant code paths, but I suspect we'll find that the result is "we were right". I suggest using this bug for large classes of trivial things (e.g. "test-code that didn't check for OOM properly"), and opening separate bugs for issues that look more serious (e.g. the include_dir thing you fixed already). If you find potential security issues, please report them as separate non-public bugs. I didn't see any in a first pass through the Coverity issues, but I didn't read through all the Coverity hits either. Created attachment 115234 [details] [review] dbus_test_tool_spam: Fix 'variable random_sizes going out of scope leaks the storage it points to' (CID 54761) Created attachment 115235 [details] [review] dbus_test_tool_spam: Fix 'variable payload going out of scope leaks the storage it points to' (CID 54759) _dbus_hash_test (void) contains several resouces leaks, which seems to be not easy to fix. CID 54730 (#3 of 3): Resource leak (RESOURCE_LEAK)49. leaked_storage: Variable value going out of scope leaks the storage it points to. CID 54740 (#6 of 6): Resource leak (RESOURCE_LEAK)71. leaked_storage: Variable key going out of scope leaks the storage it points to. Created attachment 115236 [details] [review] test_command_line_internal: Fix 'variable shell_argv going out of scope leaks the storage it points to' (CID 54751) Created attachment 115237 [details] [review] test_remove_directory: Fix 'variable iter going out of scope leaks the storage it points to' (CID 54729) Tried using the term 'Coverity-ID:' in the first two patches, instead of 'Reported by Coverity:' - not sure, which one is better. (In reply to Ralf Habacker from comment #34) > _dbus_hash_test (void) contains several resouces leaks, which seems to be > not easy to fix. > > CID 54730 (#3 of 3): Resource leak (RESOURCE_LEAK)49. leaked_storage: > Variable value going out of scope leaks the storage it points to. > > CID 54740 (#6 of 6): Resource leak (RESOURCE_LEAK)71. leaked_storage: > Variable key going out of scope leaks the storage it points to. ... but looks important. Remember to saw several memory leak messages on windows after running hash test. For all coverige resource leaks issues (except CID 54730 and 54740) patches has been provided. Comment on attachment 115234 [details] [review] dbus_test_tool_spam: Fix 'variable random_sizes going out of scope leaks the storage it points to' (CID 54761) Review of attachment 115234 [details] [review]: ----------------------------------------------------------------- Sure Comment on attachment 115235 [details] [review] dbus_test_tool_spam: Fix 'variable payload going out of scope leaks the storage it points to' (CID 54759) Review of attachment 115235 [details] [review]: ----------------------------------------------------------------- OK Comment on attachment 115236 [details] [review] test_command_line_internal: Fix 'variable shell_argv going out of scope leaks the storage it points to' (CID 54751) Review of attachment 115236 [details] [review]: ----------------------------------------------------------------- OK Comment on attachment 115237 [details] [review] test_remove_directory: Fix 'variable iter going out of scope leaks the storage it points to' (CID 54729) Review of attachment 115237 [details] [review]: ----------------------------------------------------------------- OK Comment on attachment 115234 [details] [review] dbus_test_tool_spam: Fix 'variable random_sizes going out of scope leaks the storage it points to' (CID 54761) committed to master Comment on attachment 115235 [details] [review] dbus_test_tool_spam: Fix 'variable payload going out of scope leaks the storage it points to' (CID 54759) committed to master Comment on attachment 115236 [details] [review] test_command_line_internal: Fix 'variable shell_argv going out of scope leaks the storage it points to' (CID 54751) committed to master Comment on attachment 115237 [details] [review] test_remove_directory: Fix 'variable iter going out of scope leaks the storage it points to' (CID 54729) committed to master Created attachment 115246 [details] [review] test_command_line_internal: Fix variable original_argv going out of scope leaks the storage it points to (CID 60588) Comment on attachment 115246 [details] [review] test_command_line_internal: Fix variable original_argv going out of scope leaks the storage it points to (CID 60588) Review of attachment 115246 [details] [review]: ----------------------------------------------------------------- Looks good. > Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk> Please don't add this until/unless it is true. Comment on attachment 115246 [details] [review] test_command_line_internal: Fix variable original_argv going out of scope leaks the storage it points to (CID 60588) committed to master Created attachment 115270 [details] [review] dbus_server_set_auth_mechanisms: Fix returning without unlocking server->mutex->lock (CID 54749) Comment on attachment 115270 [details] [review] dbus_server_set_auth_mechanisms: Fix returning without unlocking server->mutex->lock (CID 54749) Review of attachment 115270 [details] [review]: ----------------------------------------------------------------- OK for master, and also 1.8 if you think it's likely to affect real people. (I suspect the answer is "no it won't" though, because the only reason it can fail is OOM.) (In reply to Simon McVittie from comment #52) > Comment on attachment 115270 [details] [review] [review] > dbus_server_set_auth_mechanisms: Fix returning without unlocking > server->mutex->lock (CID 54749) > > Review of attachment 115270 [details] [review] [review]: > ----------------------------------------------------------------- > > OK for master, and also 1.8 if you think it's likely to affect real people. > (I suspect the answer is "no it won't" though, because the only reason it > can fail is OOM.) dbus-test runs tests with limited memory to force oom conditions. If this function is called by such tests, this bug would have unwanted negative side effects on test results. While writing this I remembered the case mentioned in comment 38 where I saw memory leaks: It was the hash test running under oom condition. (In reply to Ralf Habacker from comment #53) > dbus-test runs tests with limited memory to force oom conditions. If this > function is called by such tests, this bug would have unwanted negative side > effects on test results. Sure, but our goal is to produce an IPC system that can be used as OS infrastructure, not a set of passing tests (or a clean Coverity report, for that matter). Successful tests are evidence that we're achieving our goals, rather than being a goal in their own right. If the tests fail (or leak memory, or whatever), that indicates a bug; bugs might make D-Bus a worse IPC system for its major uses (=> fix in 1.8), or they might not have a practical effect on real systems (=> fixing it in master is good, but the stable branches don't need the change). Whenever we change the stable branches, there's a chance that we make a mistake and cause a new bug. We should only take that chance if the probability of adding a bug * the expected severity of that bug is less than the severity of the bug we're fixing :-) > While writing this I remembered the case mentioned in comment 38 where I saw > memory leaks: It was the hash test running under oom condition. It wouldn't surprise me. The OOM code paths are basically never exercised on real systems, because as a first approximation, malloc() never fails. (Particularly on Linux, where the kernel normally overcommits memory, and OOM is reported via SIGKILL rather than via malloc failing.) That's why the tests do such strange things to try to exercise it. The hash-table code is also copied from somewhere else (Tcl, I think) rather than being original to libdbus, which doesn't exactly help with our level of understanding. Comment on attachment 115270 [details] [review] dbus_server_set_auth_mechanisms: Fix returning without unlocking server->mutex->lock (CID 54749) committed to master and dbus-1.8 Created attachment 115576 [details] [review] auth_set_unix_credentials: Fix calling _dbus_credentials_add_unix_uid without checking return value (CID 54722). Created attachment 115577 [details] [review] auth_set_unix_credentials: Fix calling _dbus_credentials_add_pid without checking return value (CID 54708). Created attachment 115578 [details] [review] dbus_message_demarshal: Fix calling _dbus_string_append_len without checking return value (CID 54690). Created attachment 115579 [details] [review] do_check_nonce: Fix of calling _dbus_string_append_len without checking return value (CID 54720). Created attachment 115580 [details] [review] reader_init: Initialize all fields of struct DBusTypeReader (CID 54754, 54772, 54773). Comment on attachment 115576 [details] [review] auth_set_unix_credentials: Fix calling _dbus_credentials_add_unix_uid without checking return value (CID 54722). Review of attachment 115576 [details] [review]: ----------------------------------------------------------------- Looks good for master. Not for 1.8, this is only test code. Comment on attachment 115577 [details] [review] auth_set_unix_credentials: Fix calling _dbus_credentials_add_pid without checking return value (CID 54708). Review of attachment 115577 [details] [review]: ----------------------------------------------------------------- Looks good for master. Not for 1.8, this is only test code. Comment on attachment 115578 [details] [review] dbus_message_demarshal: Fix calling _dbus_string_append_len without checking return value (CID 54690). Review of attachment 115578 [details] [review]: ----------------------------------------------------------------- Looks good, for both master and 1.8 Comment on attachment 115579 [details] [review] do_check_nonce: Fix of calling _dbus_string_append_len without checking return value (CID 54720). Review of attachment 115579 [details] [review]: ----------------------------------------------------------------- Looks good for both master and 1.8, with or without the suggested change ::: dbus/dbus-nonce.c @@ +79,5 @@ > else > { > + if (!_dbus_string_append_len (&buffer, _dbus_string_get_const_data (&p), n)) > + { > + dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL); This could just be _DBUS_SET_OOM (error); Comment on attachment 115580 [details] [review] reader_init: Initialize all fields of struct DBusTypeReader (CID 54754, 54772, 54773). Review of attachment 115580 [details] [review]: ----------------------------------------------------------------- Yes for both master and 1.8, with or without my suggested change ::: dbus/dbus-marshal-recursive.c @@ +156,5 @@ > reader->value_str = value_str; > reader->value_pos = value_pos; > + reader->array_len_offset = 0; > + reader->u.array.start_pos = 0; > + reader->klass = NULL; I'd slightly prefer to _DBUS_ZERO (*reader) at the top of this function instead, to make sure *everything* is initialized; then set only the nonzero fields according to the parameters. But this works too. Comment on attachment 115576 [details] [review] auth_set_unix_credentials: Fix calling _dbus_credentials_add_unix_uid without checking return value (CID 54722). committed to master Comment on attachment 115577 [details] [review] auth_set_unix_credentials: Fix calling _dbus_credentials_add_pid without checking return value (CID 54708). committed to master Comment on attachment 115578 [details] [review] dbus_message_demarshal: Fix calling _dbus_string_append_len without checking return value (CID 54690). committed to dbus-1.8 and master Comment on attachment 115579 [details] [review] do_check_nonce: Fix of calling _dbus_string_append_len without checking return value (CID 54720). >Comment on attachment 115579 [details] [review] [review] >This could just be _DBUS_SET_OOM (error); Not used this suggestion because all locations setting an OOM error in this file uses dbus_set_error consistently. committed to dbus-1.8 and master Comment on attachment 115580 [details] [review] reader_init: Initialize all fields of struct DBusTypeReader (CID 54754, 54772, 54773). > Comment on attachment 115580 [details] [review] [review] >I'd slightly prefer to _DBUS_ZERO (*reader) at the top of this function instead, >to make sure *everything* is initialized; Committed to dbus-1.8 and master with this suggestion, because it also covers additional struct members added in future. Comment on attachment 115580 [details] [review] reader_init: Initialize all fields of struct DBusTypeReader (CID 54754, 54772, 54773). (In reply to Ralf Habacker from comment #70) > >I'd slightly prefer to _DBUS_ZERO (*reader) at the top of this function instead, to make sure *everything* is initialized; > Committed to dbus-1.8 and master with this suggestion, because it also > covers additional struct members added in future. Unfortunately this breaks dbus' tests and real-world use, so I've reverted it. Looks like something else somewhere in the tree assumes that reader_init() will in fact not initialize the entire reader. Created attachment 115635 [details] [review] reader_init: Initialize all fields of struct DBusTypeReader (CID 54754, 54772, 54773) From: Ralf Habacker <ralf.habacker@freenet.de> This patch is based on the fix for 'Field reader.array_len_offset is uninitialized' Reported by Coverity: CID 54754, 54772, 54773: Uninitialized scalar variable (UNINIT) [smcv: also re-order how the class is set when we recurse, so that the sub-reader's class doesn't end up NULL] --- This modified version passes tests for dbus-1.8; but since it has already caused trouble, and we've never actually seen a crash attributed to this, I'm tempted to leave it out of the 1.8 branch and only apply it to master. We can cherry-pick it back into 1.8 after it has been in a 1.9.x release for a while, perhaps. (In reply to Simon McVittie from comment #72) > This modified version passes tests for dbus-1.8; but since it has already > caused trouble, and we've never actually seen a crash attributed to this, > I'm tempted to leave it out of the 1.8 branch and only apply it to master. > We can cherry-pick it back into 1.8 after it has been in a 1.9.x release for > a while, perhaps. I released 1.8 before realising I had merged this there. Oh well, it's in 1.8.18 now. Created attachment 117015 [details] [review] bus_registry_new: Assert in case of not valid context parameter to avoid potiental crashes (CID 54764). Comment on attachment 117015 [details] [review] bus_registry_new: Assert in case of not valid context parameter to avoid potiental crashes (CID 54764). Review of attachment 117015 [details] [review]: ----------------------------------------------------------------- This doesn't actually "avoid potential crashes" because an assertion failure is also a crash. However, it does "document our assumptions" in a way that both developers and Coverity can understand, so it's a good change for master. Not for 1.8, since it does not fix a bug (it only makes a difference in situations that do not actually arise in the real world). Comment on attachment 117015 [details] [review] bus_registry_new: Assert in case of not valid context parameter to avoid potiental crashes (CID 54764). committed to master Created attachment 119577 [details] [review] shell-test: Calling _dbus_string_init without checking return value (CID 60587). Created attachment 119578 [details] [review] shell-test: Calling _dbus_string_init without checking return value (CID 60587). rebased Comment on attachment 119578 [details] [review] shell-test: Calling _dbus_string_init without checking return value (CID 60587). Review of attachment 119578 [details] [review]: ----------------------------------------------------------------- OK for master. No point for dbus-1.10: this is test code, not production code, and in practice malloc() doesn't actually return NULL during testing. Comment on attachment 119578 [details] [review] shell-test: Calling _dbus_string_init without checking return value (CID 60587). has been committed to master in 2015
In _dbus_hash_test() there is
while (i < 3000)
1489 {
1490 void *value;
1491 char *key;
1492
1493 key = _dbus_strdup (keys[i]);
1494 if (key == NULL)
1495 goto out;
1496 value = _dbus_strdup ("Value!");
...
1762 if (!_dbus_hash_iter_lookup (table2,
1763 _DBUS_INT_TO_POINTER (i), TRUE, &iter))
CID 54730: Resource leak (RESOURCE_LEAK)49. leaked_storage: Variable value going out of scope leaks the storage it points to.
1764 goto out;
The problem here is that 'value' is a variable in a local loop, which could not be free'd at out: jump target.
In C++ code I would use a QScopedPointer or similar to fix this issue. In C it looks that a fix requires a major rewrite of the related code, because it is not easy to follow when value has to be free'd and when not.
As QScopedPointer equivalent there is https://github.com/Snaipe/libcsptr. Would that be an option ?
(In reply to Ralf Habacker from comment #81) > In _dbus_hash_test() This is in test code => low priority. > In C++ code I would use a QScopedPointer or similar to fix this issue. In C > it looks that a fix requires a major rewrite of the related code, because it > is not easy to follow when value has to be free'd and when not. > As QScopedPointer equivalent there is https://github.com/Snaipe/libcsptr. > Would that be an option ? No, please do not introduce new library dependencies into the reference implementation of D-Bus, doubly so if they are uncommon libraries. Given that there hasn’t been movement on this in 5 months, and the leak is in test code, let’s close this bug. Future Coverity fixes can be handled in new bug reports. Looks like D-Bus is down to 35 open issues, which is great! |
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.