Created attachment 125675 [details] [review]
[01] Remove trailing whitespaces

Created attachment 125676 [details] [review]
[02] Initialize 'klass' in |_dbus_type_reader_recurse| to NULL

Created attachment 125677 [details] [review]
[03] dbus-launch: Protect |concat2| by DBUS_ENABLE_EMBEDDED_TESTS

Created attachment 125678 [details] [review]
[04] Protect 'i' in |_handle_inotify_watch| by DBUS_ENABLE_VERBOSE_MODE

Created attachment 125679 [details] [review]
[05] Fix misleading indention to avoid respective compiler warning

Created attachment 125680 [details] [review]
[06] Protect 'orig_len' in |recover_unused_bytes| by DBUS_ENABLE_VERBOSE_MODE

Created attachment 125681 [details] [review]
[07] auth: Remove 'WaitingForOK' from client

Comment on attachment 125675 [details] [review]
[01] Remove trailing whitespaces

This patch fixes only the tailing white spaces for the files in this patch set. My editor is configured to remove them automatically. Since I had the patch file anyway, I thought I could also submit it.

Created attachment 125682 [details] [review]
[02] Initialize 'klass' in |_dbus_type_reader_recurse| to NULL (v2)

Changes since v1:

  - added |_dbus_assert| to test that klass != NULL

|_dbus_assert| is also empty in release mode, so the warning persists.

Created attachment 125683 [details] [review]
[04] Protect 'i' in |_handle_inotify_watch| by DBUS_ENABLE_VERBOSE_MODE (v2)

Changes since v1:

  - don't move 'i' next to while loop
  - protect 'i' by DBUS_ENABLE_VERBOSE_MODE

Created attachment 125684 [details] [review]
[05] Fix misleading indentation to avoid respective compiler warning (v2)

Changes since v1:

  - 'indention' -> 'indentation'

Comment on attachment 125681 [details] [review]
[07] auth: Remove 'WaitingForOK' from client

The code for 'WaitingForOK' is unused. According to the protocol spec on the DBus website, the client should enter this state when the authentication mechanism says so. (Right?)

The authentication code doesn't actually offer this functionality. When it's in 'WaitingForData' and sees an OK, it handles the OK as if it was in 'WaitingForOK'. [1] The code apparently works, but it's not strictly following the specification.

I had (incomplete) patches for fixing this, but I'm new to the code base and didn't want to start with the 'interesting' stuff right away. :)

[1] https://cgit.freedesktop.org/dbus/dbus/tree/dbus/dbus-auth.c?id=4e06dcf35bc8f2e3d89bbcd40ac87f4fae0085ae#n2022

(In reply to Thomas Zimmermann from comment #8)
> This patch fixes only the tailing white spaces for the files in this patch
> set. My editor is configured to remove them automatically.

Thanks, but I would prefer not to apply this. Doing whitespace correctly (consistent use of spaces vs. tabs, no trailing whitespace) is a good thing in general, but dbus has too much historical whitespace damage for fixing it to be a small change.


We've fixed several security vulnerabilities in the past, which means there are probably more, so I'm reluctant to make it harder to cherry-pick fixes from master to older branches.

(In reply to Thomas Zimmermann from comment #9)
> |_dbus_assert| is also empty in release mode, so the warning persists.

To be clear, do you mean the warning persists unless you also initialize klass, which is why you have also done that?

(In reply to Thomas Zimmermann from comment #12)
> The code for 'WaitingForOK' is unused. According to the protocol spec on the
> DBus website, the client should enter this state when the authentication
> mechanism says so. (Right?)

The protocol specification is an abstract description of how a D-Bus client should behave, and a correct D-Bus client doesn't necessarily need to have precisely the same state machine, as long as it has compatible behaviour. I'm not particularly surprised that the two are not the same; this is not a frequently modified part of the code, or a particularly well-understood part of the specification.

If the result of switching to the specification's behaviour would not interoperate with deployed dbus-daemon implementations, then it's about 10 years too late to fix it, and we should consider fixing the specification to match the reference implementation instead.

> I had (incomplete) patches for fixing this, but I'm new to the code base and
> didn't want to start with the 'interesting' stuff right away. :)

We should fix it properly or not at all. If the warning is preventing you from compiling dbus, I think the right thing to do is to open a separate bug for investigating what is going on here, and silence the warning with _DBUS_UNUSED (or whatever we called our __attribute__((__unused__)) macro), with a comment pointing to the bug.

(In reply to Simon McVittie from comment #13)
> (In reply to Thomas Zimmermann from comment #8)
> > This patch fixes only the tailing white spaces for the files in this patch
> > set. My editor is configured to remove them automatically.
> 
> Thanks, but I would prefer not to apply this.

No problem. I'll check if the patch set needs a rebase to apply without the white-space fixes.

(In reply to Simon McVittie from comment #14)
> (In reply to Thomas Zimmermann from comment #9)
> > |_dbus_assert| is also empty in release mode, so the warning persists.
> 
> To be clear, do you mean the warning persists unless you also initialize
> klass, which is why you have also done that?

Yes, the warning persists.

(In reply to Simon McVittie from comment #15)

> If the result of switching to the specification's behaviour would not
> interoperate with deployed dbus-daemon implementations, then it's about 10
> years too late to fix it, and we should consider fixing the specification to
> match the reference implementation instead.

I agree. Just out of interest: how could one test this? You probably don't run a historical DBus releases for testing?

OTOH 'git blame' says that most of the code was written in the early 2000s, so most of the releases since then should ship the same code as the one in 'master'.

> > I had (incomplete) patches for fixing this, but I'm new to the code base and
> > didn't want to start with the 'interesting' stuff right away. :)
> 
> We should fix it properly or not at all.

Yes. I had that incomplete patch, but realized that it would be too invasive for a simple clean up. Maybe let's open a follow-up bug; in case someone wants to investigate.

(In reply to Thomas Zimmermann from comment #18)
> I agree. Just out of interest: how could one test this? You probably don't
> run a historical DBus releases for testing?

In practice no, although it would be possible to compile a new dbus on an old-ish Linux distribution (Debian oldstable or something) and connect to the system's dbus-daemon.

> OTOH 'git blame' says that most of the code was written in the early 2000s,
> so most of the releases since then should ship the same code as the one in
> 'master'.

Yes. Your change doesn't actually affect the server-side, and presumably your incomplete patches for fixing this properly won't either; so the existing test coverage is likely good enough.

> Maybe let's open a follow-up bug; in case someone
> wants to investigate.

https://bugs.freedesktop.org/show_bug.cgi?id=97298

(In reply to Thomas Zimmermann from comment #16)
> No problem. I'll check if the patch set needs a rebase to apply without the
> white-space fixes.

One patch does, but I've done that now. Testing in progress.

I've applied all these, except Attachment #125675 [details] (whitespace) and Attachment #125681 [details] (WaitingForOK) which I'm not going to apply. Fixed in git for 1.11.4, thanks.

Attachment #125680 [details] needed a small amount of adjustment to apply when whitespace damage hadn't been fixed. I included whitespace fixes for the lines adjacent to those affected by this patch, because that isn't going to make cherry-picking noticeably more difficult.

(We usually fix trailing whitespace and hard tabs opportunistically, if they're in a line we are modifying anyway or an adjacent blank line, with the same justification.)

I removed the |vertical bars| around function names in the commit messages; I'm not sure where that convention comes from, but it isn't one that we use in this project. The closest we get to a special convention for function names is that we sometimes add () (e.g. "_dbus_setenv()") if we want to be unambiguous: that's the syntax used to link to a function in Doxygen.

Thank you!