Bug 90021 - Coverity scan fixes
Summary: Coverity scan fixes
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-13 20:58 UTC by Ralf Habacker
Modified: 2016-10-01 11:57 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
CID 54854: Dereference null return value (NULL_RETURNS) (706 bytes, patch)
2015-04-13 20:59 UTC, Ralf Habacker
Details | Splinter Review
CID 54846: Dereference null return value (NULL_RETURNS) (706 bytes, patch)
2015-04-13 20:59 UTC, Ralf Habacker
Details | Splinter Review
CID 54854: Dereference null return value (NULL_RETURNS) (685 bytes, patch)
2015-04-13 21:26 UTC, Ralf Habacker
Details | Splinter Review
CID 54766: Resource leak (RESOURCE_LEAK). (1.64 KB, patch)
2015-04-14 08:03 UTC, Ralf Habacker
Details | Splinter Review
Unify return value handling of BUS_CONNECTION_DATA(). (2.63 KB, patch)
2015-04-14 21:25 UTC, Ralf Habacker
Details | Splinter Review
CID 54744: Dereference after null check (FORWARD_NULL) (799 bytes, patch)
2015-04-14 21:51 UTC, Ralf Habacker
Details | Splinter Review
Always assert that BUS_CONNECTION_DATA() returns non-NULL (3.53 KB, patch)
2015-04-16 12:25 UTC, Simon McVittie
Details | Splinter Review
dbus_test_tool_spam: Fix 'variable random_sizes going out of scope leaks the storage it points to' (CID 54761) (1.04 KB, patch)
2015-04-20 19:46 UTC, Ralf Habacker
Details | Splinter Review
dbus_test_tool_spam: Fix 'variable payload going out of scope leaks the storage it points to' (CID 54759) (815 bytes, patch)
2015-04-20 19:50 UTC, Ralf Habacker
Details | Splinter Review
test_command_line_internal: Fix 'variable shell_argv going out of scope leaks the storage it points to' (CID 54751) (1.25 KB, patch)
2015-04-20 20:17 UTC, Ralf Habacker
Details | Splinter Review
test_remove_directory: Fix 'variable iter going out of scope leaks the storage it points to' (CID 54729) (812 bytes, patch)
2015-04-20 20:17 UTC, Ralf Habacker
Details | Splinter Review
test_command_line_internal: Fix variable original_argv going out of scope leaks the storage it points to (CID 60588) (1.04 KB, patch)
2015-04-21 12:15 UTC, Ralf Habacker
Details | Splinter Review
dbus_server_set_auth_mechanisms: Fix returning without unlocking server->mutex->lock (CID 54749) (855 bytes, patch)
2015-04-22 12:58 UTC, Ralf Habacker
Details | Splinter Review
auth_set_unix_credentials: Fix calling _dbus_credentials_add_unix_uid without checking return value (CID 54722). (1.06 KB, patch)
2015-05-06 07:24 UTC, Ralf Habacker
Details | Splinter Review
auth_set_unix_credentials: Fix calling _dbus_credentials_add_pid without checking return value (CID 54708). (1.07 KB, patch)
2015-05-06 07:25 UTC, Ralf Habacker
Details | Splinter Review
dbus_message_demarshal: Fix calling _dbus_string_append_len without checking return value (CID 54690). (1018 bytes, patch)
2015-05-06 07:25 UTC, Ralf Habacker
Details | Splinter Review
do_check_nonce: Fix of calling _dbus_string_append_len without checking return value (CID 54720). (1.14 KB, patch)
2015-05-06 07:26 UTC, Ralf Habacker
Details | Splinter Review
reader_init: Initialize all fields of struct DBusTypeReader (CID 54754, 54772, 54773). (2.20 KB, patch)
2015-05-06 07:26 UTC, Ralf Habacker
Details | Splinter Review
reader_init: Initialize all fields of struct DBusTypeReader (CID 54754, 54772, 54773) (4.34 KB, patch)
2015-05-08 14:56 UTC, Simon McVittie
Details | Splinter Review
bus_registry_new: Assert in case of not valid context parameter to avoid potiental crashes (CID 54764). (1.44 KB, patch)
2015-07-09 11:19 UTC, Ralf Habacker
Details | Splinter Review
shell-test: Calling _dbus_string_init without checking return value (CID 60587). (1.34 KB, patch)
2015-11-11 23:42 UTC, Ralf Habacker
Details | Splinter Review
shell-test: Calling _dbus_string_init without checking return value (CID 60587). (1.35 KB, patch)
2015-11-11 23:45 UTC, Ralf Habacker
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Ralf Habacker 2015-04-13 20:58:09 UTC
DBus source code has been scanned with coverity scan tool https://scan.coverity.com/projects/4774. This bug is intended to collect related fixes.
Comment 1 Ralf Habacker 2015-04-13 20:59:14 UTC
Created attachment 115063 [details] [review]
CID 54854: Dereference null return value (NULL_RETURNS)
Comment 2 Ralf Habacker 2015-04-13 20:59:32 UTC
Created attachment 115064 [details] [review]
CID 54846: Dereference null return value (NULL_RETURNS)
Comment 3 Simon McVittie 2015-04-13 21:03:33 UTC
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.
Comment 4 Simon McVittie 2015-04-13 21:05:05 UTC
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.
Comment 5 Ralf Habacker 2015-04-13 21:26:52 UTC
Created attachment 115065 [details] [review]
CID 54854: Dereference null return value (NULL_RETURNS)

upload correct patch
Comment 6 Ralf Habacker 2015-04-13 22:10:36 UTC
(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.
Comment 7 Ralf Habacker 2015-04-14 08:03:52 UTC
Created attachment 115068 [details] [review]
CID 54766: Resource leak (RESOURCE_LEAK).
Comment 8 Simon McVittie 2015-04-14 10:46:44 UTC
(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 9 Simon McVittie 2015-04-14 10:49:36 UTC
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 10 Ralf Habacker 2015-04-14 17:52:09 UTC
Comment on attachment 115068 [details] [review]
CID 54766: Resource leak (RESOURCE_LEAK).

committed to master
Comment 11 Ralf Habacker 2015-04-14 21:14:46 UTC
(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.
Comment 12 Ralf Habacker 2015-04-14 21:25:00 UTC
Created attachment 115072 [details] [review]
Unify return value handling of BUS_CONNECTION_DATA().
Comment 13 Ralf Habacker 2015-04-14 21:35:02 UTC
(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.
Comment 14 Ralf Habacker 2015-04-14 21:43:43 UTC
(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.
Comment 15 Ralf Habacker 2015-04-14 21:51:40 UTC
Created attachment 115073 [details] [review]
CID 54744: Dereference after null check (FORWARD_NULL)
Comment 16 Ralf Habacker 2015-04-14 22:06:17 UTC
(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.
Comment 17 Simon McVittie 2015-04-15 09:34:53 UTC
(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 18 Simon McVittie 2015-04-15 09:37:21 UTC
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 19 Simon McVittie 2015-04-15 09:42:21 UTC
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
Comment 20 Ralf Habacker 2015-04-15 10:27:40 UTC
(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.
Comment 21 Ralf Habacker 2015-04-15 10:32:14 UTC
(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.
Comment 22 Ralf Habacker 2015-04-15 11:19:56 UTC
(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".
Comment 23 Simon McVittie 2015-04-15 16:04:44 UTC
(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.
Comment 24 Ralf Habacker 2015-04-15 19:08:43 UTC
(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.
Comment 25 Simon McVittie 2015-04-16 10:32:38 UTC
(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.)
Comment 26 Simon McVittie 2015-04-16 10:39:18 UTC
(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 27 Simon McVittie 2015-04-16 12:17:09 UTC
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 28 Simon McVittie 2015-04-16 12:18:38 UTC
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]
Comment 29 Simon McVittie 2015-04-16 12:25:31 UTC
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.
Comment 30 Ralf Habacker 2015-04-16 14:14:29 UTC
(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 31 Simon McVittie 2015-04-17 12:12:23 UTC
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.
Comment 32 Ralf Habacker 2015-04-20 19:46:37 UTC
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)
Comment 33 Ralf Habacker 2015-04-20 19:50:45 UTC
Created attachment 115235 [details] [review]
dbus_test_tool_spam: Fix 'variable payload going out of scope leaks the storage it points to' (CID 54759)
Comment 34 Ralf Habacker 2015-04-20 20:10:45 UTC
_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.
Comment 35 Ralf Habacker 2015-04-20 20:17:01 UTC
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)
Comment 36 Ralf Habacker 2015-04-20 20:17:26 UTC
Created attachment 115237 [details] [review]
test_remove_directory: Fix 'variable iter going out of scope leaks the storage it points to' (CID 54729)
Comment 37 Ralf Habacker 2015-04-20 20:20:35 UTC
Tried using the term 'Coverity-ID:' in the first two patches, instead of 'Reported by Coverity:' - not sure, which one is better.
Comment 38 Ralf Habacker 2015-04-20 20:21:37 UTC
(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.
Comment 39 Ralf Habacker 2015-04-20 20:26:23 UTC
For all coverige resource leaks issues (except CID 54730 and 54740) patches has been provided.
Comment 40 Simon McVittie 2015-04-21 08:51:21 UTC
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 41 Simon McVittie 2015-04-21 08:51:33 UTC
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 42 Simon McVittie 2015-04-21 08:51:46 UTC
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 43 Simon McVittie 2015-04-21 08:52:04 UTC
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 44 Ralf Habacker 2015-04-21 10:41:38 UTC
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 45 Ralf Habacker 2015-04-21 10:41:46 UTC
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 46 Ralf Habacker 2015-04-21 10:41:54 UTC
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 47 Ralf Habacker 2015-04-21 10:42:04 UTC
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
Comment 48 Ralf Habacker 2015-04-21 12:15:31 UTC
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 49 Simon McVittie 2015-04-21 14:00:46 UTC
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 50 Ralf Habacker 2015-04-21 14:59:37 UTC
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
Comment 51 Ralf Habacker 2015-04-22 12:58:35 UTC
Created attachment 115270 [details] [review]
dbus_server_set_auth_mechanisms: Fix returning without unlocking server->mutex->lock (CID 54749)
Comment 52 Simon McVittie 2015-04-22 14:01:15 UTC
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.)
Comment 53 Ralf Habacker 2015-04-22 14:10:47 UTC
(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.
Comment 54 Simon McVittie 2015-04-22 15:27:50 UTC
(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 55 Ralf Habacker 2015-04-28 21:09:49 UTC
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
Comment 56 Ralf Habacker 2015-05-06 07:24:57 UTC
Created attachment 115576 [details] [review]
auth_set_unix_credentials: Fix calling _dbus_credentials_add_unix_uid without checking return value (CID 54722).
Comment 57 Ralf Habacker 2015-05-06 07:25:17 UTC
Created attachment 115577 [details] [review]
auth_set_unix_credentials: Fix calling _dbus_credentials_add_pid without checking return value (CID 54708).
Comment 58 Ralf Habacker 2015-05-06 07:25:41 UTC
Created attachment 115578 [details] [review]
dbus_message_demarshal: Fix calling _dbus_string_append_len without checking return value (CID 54690).
Comment 59 Ralf Habacker 2015-05-06 07:26:03 UTC
Created attachment 115579 [details] [review]
do_check_nonce: Fix of calling _dbus_string_append_len without checking return value (CID 54720).
Comment 60 Ralf Habacker 2015-05-06 07:26:23 UTC
Created attachment 115580 [details] [review]
reader_init: Initialize all fields of struct DBusTypeReader (CID 54754, 54772, 54773).
Comment 61 Simon McVittie 2015-05-06 09:26:58 UTC
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 62 Simon McVittie 2015-05-06 09:27:24 UTC
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 63 Simon McVittie 2015-05-06 09:28:13 UTC
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 64 Simon McVittie 2015-05-06 09:29:32 UTC
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 65 Simon McVittie 2015-05-06 09:40:43 UTC
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 66 Ralf Habacker 2015-05-06 10:15:42 UTC
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 67 Ralf Habacker 2015-05-06 10:15:55 UTC
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 68 Ralf Habacker 2015-05-06 10:16:26 UTC
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 69 Ralf Habacker 2015-05-06 10:20:36 UTC
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 70 Ralf Habacker 2015-05-06 10:22:40 UTC
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 71 Simon McVittie 2015-05-08 14:42:04 UTC
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.
Comment 72 Simon McVittie 2015-05-08 14:56:40 UTC
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.
Comment 73 Simon McVittie 2015-05-14 13:40:32 UTC
(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.
Comment 74 Ralf Habacker 2015-07-09 11:19:24 UTC
Created attachment 117015 [details] [review]
bus_registry_new: Assert in case of not valid context parameter to avoid potiental crashes (CID 54764).
Comment 75 Simon McVittie 2015-07-13 10:02:25 UTC
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 76 Ralf Habacker 2015-07-13 12:36:06 UTC
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
Comment 77 Ralf Habacker 2015-11-11 23:42:52 UTC
Created attachment 119577 [details] [review]
shell-test: Calling _dbus_string_init without checking  return value (CID 60587).
Comment 78 Ralf Habacker 2015-11-11 23:45:44 UTC
Created attachment 119578 [details] [review]
shell-test: Calling _dbus_string_init without checking  return value (CID 60587).

rebased
Comment 79 Simon McVittie 2015-11-12 10:14:27 UTC
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 80 Ralf Habacker 2016-05-29 20:58:02 UTC
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
Comment 81 Ralf Habacker 2016-05-29 21:08:45 UTC
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 ?
Comment 82 Simon McVittie 2016-05-30 09:40:48 UTC
(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.
Comment 83 Philip Withnall 2016-10-01 11:57:27 UTC
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.