Created attachment 38279 [details] [review]
Handle failure to allocate error message in _read_subprocess_line_argv

Created attachment 38280 [details] [review]
Check return value of XGetWindowProperty in x11_get_address

Created attachment 38281 [details] [review]
Handle failure to hex encode in handle_server_data_anonymous_mech

Created attachment 38282 [details] [review]
Verify that getsockname succeeded in _dbus_listen_tcp_socket

Review of attachment 38279 [details] [review]:

Can you report an OOM situation instead of saying that spawning failed?

Review of attachment 38280 [details] [review]:

Agreed.

Review of attachment 38281 [details] [review]:

Agreed.

Review of attachment 38282 [details] [review]:

Agreed, but check that result == -1.

Created attachment 38840 [details] [review]
Handle failure to allocate error message in _read_subprocess_line_argv #2

(In reply to comment #5)
> Review of attachment 38279 [details] [review]:
> 
> Can you report an OOM situation instead of saying that spawning failed?

Updated.

Created attachment 38841 [details] [review]
Verify that getsockname succeeded in _dbus_listen_tcp_socket #2

(In reply to comment #8)
> Review of attachment 38282 [details] [review]:
> 
> Agreed, but check that result == -1.

Indeed, gotta be careful not to confuse error codes of the two functions. Updated.

Created attachment 38906 [details] [review]
Free session file early in dbus-launch

Created attachment 38907 [details] [review]
Free listen_fd in the error case

Created attachment 38908 [details] [review]
Free envvar and argv in dbus-launch if OOM

Review of attachment 38281 [details] [review]:

::: dbus/dbus-auth.c
@@ +1210,3 @@
+            if (_dbus_string_init (&encoded))
+              {
+                _dbus_string_hex_encode (&plaintext, 0, &encoded, 0);

I think 'encoded' gets leaked here; we should free it after the _dbus_verbose() call.

Created attachment 39582 [details] [review]
Handle failure to hex encode in handle_server_data_anonymous_mech #2

(In reply to comment #14)
> Review of attachment 38281 [details] [review]:
> I think 'encoded' gets leaked here; we should free it after the _dbus_verbose()
> call.

You're right. Updated the patch.

Review of attachment 38280 [details] [review]:

r+ from me too, applied to master as 79b4e478 for either 1.4.4 or 1.5.0.

Review of attachment 38840 [details] [review]:

Looks good, commited as 14be9f73.

Review of attachment 38841 [details] [review]:

committed, 68b1d6ad

Review of attachment 38906 [details] [review]:

Applied, 916620ea

Review of attachment 38908 [details] [review]:

review- for this one.

::: dbus-1.3.1/tools/dbus-launch.c
@@ +714,3 @@
+          {
+            free (envvar);
+            free (argv);

No, that frees the argument argv, which we don't own. You mean args (and if argv was const, the compiler wouldn't have let us get this wrong).

There's another similar case just above for OOM while allocating args[0], which Coverity didn't spot because it doesn't know that xstrdup() is malloc-like.

All of this is a bit academic since pass_info() can't return - it either self-destructs via execvp() or calls exit().

Review of attachment 39582 [details] [review]:

The commit name is a bit wrong; the failure case is OOM, and success of hex-encoding still isn't checked here.

This seems a very tenuous situation:

- we're the server
- the client authenticates as anonymous
- the client supplies an arbitrary string for us to log, which might be an email address or not, but is meant to be UTF-8
- the client actually gives us non-UTF8
- we hex-encode something like "D-Bus 1.4.3" as 442d42757320312e342e33, and output "server: try '442d42757320312e342e33'" as verbose output

It's not at all clear to me why we need to verbose-output anything at all here.

FWIW, the corresponding client-side sends the hex-encoding of "libdbus 1.4.3". Havoc wrote:

   * We just send the dbus implementation info, like a user-agent or
   * something, because... why not. There's nothing guaranteed here
   * though, we could change it later.

To be honest I'd be inclined to fix this by just deleting the whole {} block containing @plaintext and @encoded.

Review of attachment 38907 [details] [review]:

I think a more comprehensible fix would be to centralize the resource-freeing by replacing the "return -1" with "goto failed", instead of adding the dbus_free call above it.

However, this patch is equivalent to that, strictly speaking, because at that point in the function, ai has already been freed and NULLed, and there are no file descriptors that need closing. I've applied it to master, and I'll propose my version as an extra patch.

Created attachment 42166 [details] [review]
dbus-launch: pass_info: always free strings on OOM

This doesn't really do anything, because we're about to exit anyway, but
it placates static analysis tools.

---

Regarding Comment #22:

> I'll propose my version as an extra patch.

r+ from wjt and pushed.

Created attachment 42167 [details] [review]
handle_server_data_anonymous_mech: remove unnecessary debug output

(In reply to comment #21)
> To be honest I'd be inclined to fix this by just deleting the whole {} block
> containing @plaintext and @encoded.

So, yeah, that.

Doing a malloc and a hex-encoding pass just to produce a _dbus_verbose
message (i.e. a message that, in practice, nobody will see) seems like
overkill, and this block had incorrect error handling (not checking the
result of _dbus_string_init) which upsets static analysis tools.

Two remaining patches are available in gitweb (see URL), reviews would be appreciated.

The handle_server_data_anonymous_mech patch is a potential crash, albeit vanishingly unlikely.

The other patch is entirely academic, but it shuts up static analysis tools (i.e. makes it easier to see the real bugs). We could merge it, or not.

Created attachment 42904 [details] [review]
_dbus_listen_tcp_socket: avoid leaking listen_fd in unlikely circumstances

Here's another patch, this time for an unlikely memory leak. Again, review would be appreciated.

Review of attachment 42166 [details] [review]:

OK to me, not a dbus dev though

Review of attachment 42167 [details] [review]:

OK to me, not a dbus dev though

(In reply to comment #27)
> OK to me, not a dbus dev though

(In reply to comment #28)
> OK to me, not a dbus dev though

Both applied in dbus-1.4 for 1.4.10, and will be merged to master for 1.5.2, based on the new review policy that nobody actually objected to. :-)

(In reply to comment #26)
> _dbus_listen_tcp_socket: avoid leaking listen_fd in unlikely circumstances

Still to be reviewed.

Comment on attachment 42904 [details] [review]
_dbus_listen_tcp_socket: avoid leaking listen_fd in unlikely circumstances

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

Yup, this looks fine.

Fixed in git for 1.4.18, 1.5.10