Bug 88810 - GLib-style "installed tests" for D-Bus, and better coverage
Summary: GLib-style "installed tests" for D-Bus, and better coverage
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: Other All
: medium enhancement
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review?
Keywords: patch
Depends on:
Blocks: 57952
  Show dependency treegraph
 
Reported: 2015-01-26 20:29 UTC by Simon McVittie
Modified: 2015-02-03 18:25 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
[PATCH 1/6] Factor out some utility functions from test/dbus-daemon* (22.96 KB, patch)
2015-01-26 20:35 UTC, Simon McVittie
Details | Splinter Review
[PATCH 2/6] Generate test configuration files via build-time sed, not configure (4.50 KB, patch)
2015-01-26 20:36 UTC, Simon McVittie
Details | Splinter Review
[PATCH 3/6] test: implement GLib-style "installed tests" (4.12 KB, patch)
2015-01-26 20:36 UTC, Simon McVittie
Details | Splinter Review
[PATCH 4/6] Add infrastructure to run bits of tests under an alternative uid (8.63 KB, patch)
2015-01-26 20:37 UTC, Simon McVittie
Details | Splinter Review
[PATCH 5/6] Add a test for uid-controlled permissions (10.75 KB, patch)
2015-01-26 20:37 UTC, Simon McVittie
Details | Splinter Review
[PATCH 6/6] bus driver: factor out bus_driver_check_caller_is_privileged, and allow root (6.76 KB, patch)
2015-01-26 20:38 UTC, Simon McVittie
Details | Splinter Review
Fix distcheck (976 bytes, patch)
2015-01-26 21:16 UTC, Simon McVittie
Details | Splinter Review
[01/11] Bump required GLib version to 2.36 (2.29 KB, patch)
2015-02-02 19:16 UTC, Simon McVittie
Details | Splinter Review
[02/11] Factor out some utility functions from test/dbus-daemon* (25.67 KB, patch)
2015-02-02 19:17 UTC, Simon McVittie
Details | Splinter Review
[03/11] Generate test configuration files via build-time sed, not configure (5.04 KB, patch)
2015-02-02 19:18 UTC, Simon McVittie
Details | Splinter Review
[04/11] test: implement GLib-style "installed tests" (4.09 KB, patch)
2015-02-02 19:19 UTC, Simon McVittie
Details | Splinter Review
[05/11] Add infrastructure to run bits of tests under an alternative uid (9.86 KB, patch)
2015-02-02 19:20 UTC, Simon McVittie
Details | Splinter Review
[06/11] Add a test for uid-controlled permissions (11.04 KB, patch)
2015-02-02 19:21 UTC, Simon McVittie
Details | Splinter Review
[07/11] bus driver: factor out bus_driver_check_caller_is_privileged, and allow root (7.35 KB, patch)
2015-02-02 19:22 UTC, Simon McVittie
Details | Splinter Review
[08/11] bus: put the printf attribute in the header where it will do more good (1.81 KB, patch)
2015-02-02 19:23 UTC, Simon McVittie
Details | Splinter Review
[09/11] bus_context_log_literal: add simplified version of bus_context_log (2.62 KB, patch)
2015-02-02 19:23 UTC, Simon McVittie
Details | Splinter Review
[10/11] _dbus_set_error_valist: add (2.36 KB, patch)
2015-02-02 19:23 UTC, Simon McVittie
Details | Splinter Review
[11/11] bus_context_log_and_set_error: add and use (6.39 KB, patch)
2015-02-02 19:24 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon McVittie 2015-01-26 20:29:46 UTC
We can get better integration test coverage if we install configuration files for the tests, and run them once with the system's session.conf (tests that require special configuration are skipped), and once with the special configuration files.

I'm doing the former in Debian's autopkgtest environment, but not yet the latter.

My particular interest in landing this is that I would like to be able to do a regression test for bus_driver_check_caller_is_privileged(), which requires running the test as root so it can setuid(). That's unsuitable for "make check", but can be automated in something like autopkgtest.
Comment 1 Simon McVittie 2015-01-26 20:35:45 UTC
Created attachment 112842 [details] [review]
[PATCH 1/6] Factor out some utility functions from test/dbus-daemon*

In the process, make test_kill_pid() safer: do not try to terminate
more than one pid, or the NULL handle.
Comment 2 Simon McVittie 2015-01-26 20:36:09 UTC
Created attachment 112843 [details] [review]
[PATCH 2/6] Generate test configuration files via build-time sed, not  configure

This means we can generate a version that works when installed,
from the same source files.
Comment 3 Simon McVittie 2015-01-26 20:36:33 UTC
Created attachment 112844 [details] [review]
[PATCH 3/6] test: implement GLib-style "installed tests"

We run each test twice:

* once with the system's session.conf, as an integration test
  (test-cases that need a special configuration are automatically
  skipped)
* once with our special test configuration files, which provide better
  coverage
Comment 4 Simon McVittie 2015-01-26 20:37:31 UTC
Created attachment 112845 [details] [review]
[PATCH 4/6] Add infrastructure to run bits of tests under an  alternative uid

---

This is not going to be enabled by default under either "make check" or installed-tests, because it requires running individual tests as root, but sandboxed environments like autopkgtest can use it selectively for better coverage.
Comment 5 Simon McVittie 2015-01-26 20:37:58 UTC
Created attachment 112846 [details] [review]
[PATCH 5/6] Add a test for uid-controlled permissions

This is technical debt from mitigating CVE-2014-8148, which should
really have had a regression test at the time.
Comment 6 Simon McVittie 2015-01-26 20:38:25 UTC
Created attachment 112847 [details] [review]
[PATCH 6/6] bus driver: factor out  bus_driver_check_caller_is_privileged, and allow root

Unlike the initial mitigation for CVE-2014-8148, we now allow
uid 0 to call UpdateActivationEnvironment. There's no point in root
doing that, but there's also no reason why it's particularly bad -
if an attacker is uid 0 we've already lost - and it simplifies
use of this function for future things that do want to be callable
by root, like BecomeMonitor for #46787.
Comment 7 Simon McVittie 2015-01-26 20:40:56 UTC
Colin, I hear you like this sort of thing :-)
Comment 8 Simon McVittie 2015-01-26 21:16:26 UTC
Created attachment 112855 [details] [review]
Fix distcheck

---

Can be squashed into Attachment #112843 [details] if desired.
Comment 9 Philip Withnall 2015-02-02 09:26:33 UTC
Comment on attachment 112842 [details] [review]
[PATCH 1/6] Factor out some utility functions from test/dbus-daemon*

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

::: test/test-utils-glib.c
@@ +1,1 @@
> +#include <config.h>

Missing a copyright header which seems to be present on other files in test/.

@@ +33,5 @@
> +{
> +  GError *error = NULL;
> +  GString *address;
> +  gint address_fd;
> +  gchar *argv[] = {

This could be (const gchar * const *), I think. The @binary and @configuration parameters could be (const gchar *).

@@ +49,5 @@
> +      NULL, /* child_setup */
> +      NULL, /* user data */
> +      daemon_pid,
> +      NULL, /* child's stdin = /dev/null */
> +      &address_fd,

I think @address_fd is leaked when this function returns. Shouldn’t it be closed when the GPid is closed?

::: test/test-utils-glib.h
@@ +1,1 @@
> +#ifndef TEST_UTILS_GLIB_H

Missing a copyright header which seems to be present on other files in test/.
Comment 10 Philip Withnall 2015-02-02 09:29:36 UTC
Comment on attachment 112843 [details] [review]
[PATCH 2/6] Generate test configuration files via build-time sed, not  configure

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

Looks good to me.
Comment 11 Philip Withnall 2015-02-02 09:34:15 UTC
Comment on attachment 112844 [details] [review]
[PATCH 3/6] test: implement GLib-style "installed tests"

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

::: test/Makefile.am
@@ +457,5 @@
> +$(installable_test_meta): %.test: % Makefile
> +	$(AM_V_GEN) ( \
> +		echo '[Test]'; \
> +		echo 'Type=session'; \
> +		echo 'Exec=env DBUS_TEST_HOME=$$(pwd)/home.tmp $(testexecdir)/$*'; \

Should this be $(DESTDIR)$(testexecdir)/$*?
Comment 12 Philip Withnall 2015-02-02 09:42:32 UTC
Comment on attachment 112845 [details] [review]
[PATCH 4/6] Add infrastructure to run bits of tests under an  alternative uid

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

::: test/test-utils-glib.c
@@ +75,5 @@
>        "--print-address=1", /* stdout */
>        NULL
>    };
> +#ifdef DBUS_UNIX
> +  struct passwd *pwd = NULL;

Should be const.

@@ +103,5 @@
> +          case TEST_USER_ROOT:
> +            break;
> +
> +          case TEST_USER_MESSAGEBUS:
> +            pwd = getpwnam (DBUS_USER);

Is it worth using getpwnam_r() here to avoid potential threading issues in future?

::: test/test-utils-glib.h
@@ +11,5 @@
> +    TEST_USER_ME,
> +    TEST_USER_ROOT,
> +    TEST_USER_MESSAGEBUS,
> +    TEST_USER_OTHER
> +} TestUser;

This could do with a brief doc comment. e.g. Who is OTHER?
Comment 13 Philip Withnall 2015-02-02 09:54:42 UTC
Comment on attachment 112846 [details] [review]
[PATCH 5/6] Add a test for uid-controlled permissions

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

::: test/test-utils-glib.c
@@ +278,5 @@
> +  /* For now we only do tests like this on Linux, because I don't know how
> +   * safe this use of setresuid() is on other platforms */
> +#if defined(HAVE_GETRESUID) && defined(HAVE_SETRESUID) && defined(__linux__)
> +  uid_t ruid, euid, suid;
> +  struct passwd *pwd;

const.

@@ +313,5 @@
> +          (unsigned long) ruid, (unsigned long) euid, (unsigned long) suid);
> +      return NULL;
> +    }
> +
> +  pwd = getpwnam (username);

Might want to use getpwnam_r() here.

::: test/uid-permissions.c
@@ +1,4 @@
> +/* Integration tests for the dbus-daemon's uid-based hardening
> + *
> + * Author: Simon McVittie <simon.mcvittie@collabora.co.uk>
> + * Copyright © 2010-2011 Nokia Corporation

The © sign has been mangled here, but that might just be a Bugzilla/Splinter issue.

@@ +146,5 @@
> +{
> +  dbus_error_free (&f->e);
> +  g_clear_error (&f->ge);
> +
> +  test_main_context_unref (f->ctx);

f->conn and f->daemon_pid are leaked here.
Comment 14 Philip Withnall 2015-02-02 10:03:51 UTC
Comment on attachment 112847 [details] [review]
[PATCH 6/6] bus driver: factor out  bus_driver_check_caller_is_privileged, and allow root

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

Note: I do not know enough about the intricacies of the D-Bus security model to review this patch for security. My review is purely of the code.

::: bus/driver.c
@@ +85,5 @@
> +static dbus_bool_t
> +bus_driver_check_caller_is_privileged (DBusConnection *connection,
> +                                       BusTransaction *transaction,
> +                                       DBusMessage    *message,
> +                                       DBusError      *error)

Would be nice to have a comment giving the high-level version of what this does, with some security justification (or a pointer to the justification in a bug report or ML or something). e.g. What does ‘privileged’ mean?

@@ +97,5 @@
> +
> +      /* Yes this repetition is pretty horrible, but there's no
> +       * bus_context_log_valist() or dbus_set_error_valist() or
> +       * bus_context_log_literal() or dbus_set_error_literal().
> +       */

Should be fairly easy to add one such method, and it would simplify the code here and below.

@@ +133,5 @@
> +  return TRUE;
> +#elif DBUS_WIN
> +  char *windows_sid;
> +
> +  if (!dbus_connection_get_windows_user (connection, &windows_sid))

@windows_sid is leaked on the success path.
Comment 15 Philip Withnall 2015-02-02 10:04:27 UTC
Comment on attachment 112855 [details] [review]
Fix distcheck

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

++
Comment 16 Simon McVittie 2015-02-02 12:51:36 UTC
(In reply to Philip Withnall from comment #9)
> Missing a copyright header which seems to be present on other files in test/.

Fair point, will fix. It's all from dbus-daemon.c, so it can be similarly permissive.

> This could be (const gchar * const *), I think. The @binary and
> @configuration parameters could be (const gchar *).

I'll peer at it and work out how many casts this would need. It's possible that the answer is "yeah, but no" if the resulting code is needlessly ugly - I prefer to avoid sprinkling these casts through utility code, because C doesn't have C++'s const_cast<T> which guarantees that only the constness changed.

> I think @address_fd is leaked when this function returns. Shouldn’t it be
> closed when the GPid is closed?

I think it can be closed as soon as we've read the address. I'll check.

(In reply to Philip Withnall from comment #11)
> > +		echo 'Exec=env DBUS_TEST_HOME=$$(pwd)/home.tmp $(testexecdir)/$*'; \
> 
> Should this be $(DESTDIR)$(testexecdir)/$*?

No, because when we copy $(DESTDIR)/foo to /foo (e.g. via a package build and installation), we will not change the content. It can only be correct for the final location *or* the DESTDIR staging area, not both; and for installed-tests we want the former.

You'll notice that "make installcheck" is skipped if DESTDIR is non-empty, because the final location gets hard-coded in a few places, so it wouldn't work properly in the presence of a DESTDIR.

(In reply to Philip Withnall from comment #12)
> > +  struct passwd *pwd = NULL;
> 
> Should be const.

Fair. For some reason the official prototype is that it returns non-const, but assigning a mutable pointer to a const-pointer variable is fine anyway.

> > +          case TEST_USER_MESSAGEBUS:
> > +            pwd = getpwnam (DBUS_USER);
> 
> Is it worth using getpwnam_r() here to avoid potential threading issues in
> future?

The reentrant version is more complicated to use, and these tests will fall over in a heap if you try to thread them anyway (setenv() and setresuid() manipulate process-global state), so I'm going to say "no".

In practice, libdbus is never going to be threaded behind the scenes like GDBus is, because dbus-daemon and the dbus-*util*.c files it uses are not thread-safe.

> ::: test/test-utils-glib.h
> @@ +11,5 @@
> > +    TEST_USER_ME,
> > +    TEST_USER_ROOT,
> > +    TEST_USER_MESSAGEBUS,
> > +    TEST_USER_OTHER
> > +} TestUser;
> 
> This could do with a brief doc comment. e.g. Who is OTHER?

Yeah, probably. It's hinted at in configure.ac - OTHER is any unprivileged user of the distribution integrator's choice, typically 'nobody', or maybe 'daemon' or something. The constraint is that they must fail the "is privileged for this bus" check for a bus running as 'messagebus' (except 'messagebus' also has a distribution-specific name).
Comment 17 Simon McVittie 2015-02-02 12:56:08 UTC
(In reply to Philip Withnall from comment #13)
> > +  struct passwd *pwd;
> 
> const.

OK

> > +  pwd = getpwnam (username);
> 
> Might want to use getpwnam_r() here.

As above, I'd rather not.

> > + * Copyright © 2010-2011 Nokia Corporation
> 
> The © sign has been mangled here, but that might just be a Bugzilla/Splinter
> issue.

As on the other bug, I'm pretty sure it's Splinter's fault.

> > +  test_main_context_unref (f->ctx);
> 
> f->conn and f->daemon_pid are leaked here.

Good catch, will check that.

(In reply to Philip Withnall from comment #14)
> > +static dbus_bool_t
> > +bus_driver_check_caller_is_privileged (DBusConnection *connection,
> > +                                       BusTransaction *transaction,
> > +                                       DBusMessage    *message,
> > +                                       DBusError      *error)
> 
> Would be nice to have a comment giving the high-level version of what this
> does, with some security justification (or a pointer to the justification in
> a bug report or ML or something). e.g. What does ‘privileged’ mean?

Reasonable. I stole the security model from kdbus: it's approximately "it is usually pointless to protect against this uid because they can ptrace me anyway" :-)

> > +      /* Yes this repetition is pretty horrible, but there's no
> > +       * bus_context_log_valist() or dbus_set_error_valist() or
> > +       * bus_context_log_literal() or dbus_set_error_literal().
> > +       */
> 
> Should be fairly easy to add one such method, and it would simplify the code
> here and below.

So you'd think. I spent a little while poking at dbus_set_error, but the combination of va_list and trying to be OOM-safe meant I would have had to do a lot of refactoring and possibly duplicate a bunch of code, and my assessment was that it would deter reviewers more than it would help.

If you'd be up for reviewing it, I wouldn't mind having another go, but I'd rather not have it block functionally necessary things.

> > +  if (!dbus_connection_get_windows_user (connection, &windows_sid))
> 
> @windows_sid is leaked on the success path.

Good catch, will fix.
Comment 18 Simon McVittie 2015-02-02 17:28:58 UTC
(In reply to Philip Withnall from comment #9)
> [argv] could be (const gchar * const *), I think.
> The @binary and @configuration parameters could be (const gchar *).

g_spawn_async_with_pipes() takes a gchar **, so I can only make any of those changes if I insert a non-const-correct cast. I'll attach the resulting patch and let you decide which way you think looks less bad...

> I think @address_fd is leaked when this function returns.

Indeed. 

> Shouldn’t it be closed when the GPid is closed?

It can be closed immediately.

> Missing a copyright header which seems to be present on other files in test/.

Fixed.

(In reply to Simon McVittie from comment #17)
> > const.
> 
> OK

Fixed, at the cost of yet another cast :'-(

> > > +      /* Yes this repetition is pretty horrible, but there's no
> > > +       * bus_context_log_valist() or dbus_set_error_valist() or
> > > +       * bus_context_log_literal() or dbus_set_error_literal().
> > > +       */
> > 
> > Should be fairly easy to add one such method, and it would simplify the code
> > here and below.
> 
> So you'd think. I spent a little while poking at dbus_set_error, but the
> combination of va_list and trying to be OOM-safe meant [bad things]

Actually I was mostly just confused by the fact that dbus_set_error() supports format == NULL and worried that va_start after that would be a problem - but in fact that's no worse than calling dbus_set_error() with a literal message (no % specifiers and no following arguments).

To reduce the number of malloc/free cycles, the right thing to do is actually to make a temporary DBusError on the stack, log its message, then move it into the real DBusError (which might be NULL but dbus_move_error copes with that).
Comment 19 Simon McVittie 2015-02-02 17:40:42 UTC
(In reply to Simon McVittie from comment #18)
> g_spawn_async_with_pipes() takes a gchar **, so I can only make any of those
> changes if I insert a non-const-correct cast. I'll attach the resulting
> patch and let you decide which way you think looks less bad...

Looking at the resulting patch, I think it isn't so bad.
Comment 20 Simon McVittie 2015-02-02 19:16:56 UTC
Created attachment 113053 [details] [review]
[01/11] Bump required GLib version to 2.36

This is for g_close(), which the next commit will use. It also lets us
rely on g_type_init() being a no-op (since 2.32 the type system is
always initialized by a global constructor).
Comment 21 Simon McVittie 2015-02-02 19:17:25 UTC
Created attachment 113054 [details] [review]
[02/11] Factor out some utility functions from  test/dbus-daemon*

In the process, make test_kill_pid() safer: do not try to terminate
more than one pid, or the NULL handle.

Also stop leaking the address_fd in spawn_dbus_daemon, a pre-existing
bug that was spotted by Philip Withnall during review.
Comment 22 Simon McVittie 2015-02-02 19:18:23 UTC
Created attachment 113055 [details] [review]
[03/11] Generate test configuration files via build-time sed,  not configure

---

Philip already reviewed this, as two separate patches.
Comment 23 Simon McVittie 2015-02-02 19:19:32 UTC
Created attachment 113056 [details] [review]
[04/11] test: implement GLib-style "installed tests"

---

Unchanged since Philip reviewed the previous version; I think his query is in fact a non-issue (see above).
Comment 24 Simon McVittie 2015-02-02 19:20:40 UTC
Created attachment 113057 [details] [review]
[05/11] Add infrastructure to run bits of tests under an  alternative uid

---

Now with const pointers (and casts because g_spawn_async_with_pipes() is not const-correct), and documentation for the TestUser enum.
Comment 25 Simon McVittie 2015-02-02 19:21:40 UTC
Created attachment 113058 [details] [review]
[06/11] Add a test for uid-controlled permissions

This is technical debt from mitigating CVE-2014-8148, which should
really have had a regression test at the time.

---

Now more const-correct and less leaky.
Comment 26 Simon McVittie 2015-02-02 19:22:41 UTC
Created attachment 113059 [details] [review]
[07/11] bus driver: factor out  bus_driver_check_caller_is_privileged, and allow root

---

Now with documentation, and not leaking the Windows SID. Repetitive logging/error code will be fixed in a subsequent patch.
Comment 27 Simon McVittie 2015-02-02 19:23:10 UTC
Created attachment 113060 [details] [review]
[08/11] bus: put the printf attribute in the header where it  will do more good

Now we can actually notice incorrect format strings in other
translation units.

---

New, minor fix.
Comment 28 Simon McVittie 2015-02-02 19:23:36 UTC
Created attachment 113061 [details] [review]
[09/11] bus_context_log_literal: add simplified version of  bus_context_log
Comment 29 Simon McVittie 2015-02-02 19:23:55 UTC
Created attachment 113062 [details] [review]
[10/11] _dbus_set_error_valist: add
Comment 30 Simon McVittie 2015-02-02 19:24:23 UTC
Created attachment 113063 [details] [review]
[11/11] bus_context_log_and_set_error: add and use
Comment 31 Philip Withnall 2015-02-03 13:26:33 UTC
Comment on attachment 113053 [details] [review]
[01/11] Bump required GLib version to 2.36

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

++
Comment 32 Philip Withnall 2015-02-03 13:31:16 UTC
Comment on attachment 113054 [details] [review]
[02/11] Factor out some utility functions from  test/dbus-daemon*

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

::: test/test-utils-glib.c
@@ +110,5 @@
> +
> +      g_usleep (G_USEC_PER_SEC / 10);
> +    }
> +
> +  g_close (address_fd, NULL);

I assume this means dbus-daemon gracefully ignores EPIPE?
Comment 33 Philip Withnall 2015-02-03 13:37:38 UTC
Comment on attachment 113057 [details] [review]
[05/11] Add infrastructure to run bits of tests under an  alternative uid

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

++
Comment 34 Philip Withnall 2015-02-03 13:39:13 UTC
Comment on attachment 113058 [details] [review]
[06/11] Add a test for uid-controlled permissions

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

++
Comment 35 Philip Withnall 2015-02-03 13:41:36 UTC
Comment on attachment 113059 [details] [review]
[07/11] bus driver: factor out  bus_driver_check_caller_is_privileged, and allow root

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

++
Comment 36 Philip Withnall 2015-02-03 13:43:51 UTC
Comment on attachment 113060 [details] [review]
[08/11] bus: put the printf attribute in the header where it  will do more good

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

++!
Comment 37 Philip Withnall 2015-02-03 13:45:30 UTC
Comment on attachment 113061 [details] [review]
[09/11] bus_context_log_literal: add simplified version of  bus_context_log

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

++
Comment 38 Philip Withnall 2015-02-03 13:49:42 UTC
Comment on attachment 113062 [details] [review]
[10/11] _dbus_set_error_valist: add

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

Commit message terse.

::: dbus/dbus-errors.c
@@ +378,5 @@
> +{
> +  DBusRealError *real;
> +  DBusString str;
> +
> +  _dbus_assert (name != NULL);

This doesn’t do the check:
  _dbus_return_if_error_is_set (error);
Comment 39 Philip Withnall 2015-02-03 13:51:17 UTC
Comment on attachment 113063 [details] [review]
[11/11] bus_context_log_and_set_error: add and use

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

++
Comment 40 Philip Withnall 2015-02-03 13:54:17 UTC
Updated reviews done. Any patches I haven’t explicitly re-reviewed are still ++ from before.

(In reply to Simon McVittie from comment #16)
> (In reply to Philip Withnall from comment #11)
> > > +		echo 'Exec=env DBUS_TEST_HOME=$$(pwd)/home.tmp $(testexecdir)/$*'; \
> > 
> > Should this be $(DESTDIR)$(testexecdir)/$*?
> 
> No, because when we copy $(DESTDIR)/foo to /foo (e.g. via a package build
> and installation), we will not change the content. It can only be correct
> for the final location *or* the DESTDIR staging area, not both; and for
> installed-tests we want the former.
> 
> You'll notice that "make installcheck" is skipped if DESTDIR is non-empty,
> because the final location gets hard-coded in a few places, so it wouldn't
> work properly in the presence of a DESTDIR.

Fair enough. Thanks for the explanation.

> > > +          case TEST_USER_MESSAGEBUS:
> > > +            pwd = getpwnam (DBUS_USER);
> > 
> > Is it worth using getpwnam_r() here to avoid potential threading issues in
> > future?
> 
> The reentrant version is more complicated to use, and these tests will fall
> over in a heap if you try to thread them anyway (setenv() and setresuid()
> manipulate process-global state), so I'm going to say "no".
> 
> In practice, libdbus is never going to be threaded behind the scenes like
> GDBus is, because dbus-daemon and the dbus-*util*.c files it uses are not
> thread-safe.

I thought that might be the case, but thought it warranted mentioning anyway. :-)

> > ::: test/test-utils-glib.h
> > @@ +11,5 @@
> > > +    TEST_USER_ME,
> > > +    TEST_USER_ROOT,
> > > +    TEST_USER_MESSAGEBUS,
> > > +    TEST_USER_OTHER
> > > +} TestUser;
> > 
> > This could do with a brief doc comment. e.g. Who is OTHER?
> 
> Yeah, probably. It's hinted at in configure.ac - OTHER is any unprivileged
> user of the distribution integrator's choice, typically 'nobody', or maybe
> 'daemon' or something. The constraint is that they must fail the "is
> privileged for this bus" check for a bus running as 'messagebus' (except
> 'messagebus' also has a distribution-specific name).

++ for your ‘brief’ comment!
Comment 41 Simon McVittie 2015-02-03 14:52:49 UTC
(In reply to Philip Withnall from comment #32)
> > +  g_close (address_fd, NULL);
> 
> I assume this means dbus-daemon gracefully ignores EPIPE?

Actually no; if we have MSG_NOSIGNAL, SIGPIPE is not ignored. However, dbus-daemon should not be writing anything to its stdout, other than its address + "\n" where requested (which we do, here). If it did, it would be annoying to capture the address, because you wouldn't know what was address and what was other rubbish...
Comment 42 Simon McVittie 2015-02-03 15:00:12 UTC
(In reply to Philip Withnall from comment #38)
> Comment on attachment 113062 [details] [review]
> [10/11] _dbus_set_error_valist: add
>
> Commit message terse.

Well, yes. It does what its name says: dbus_set_error(), but with a va_list :-)

> ::: dbus/dbus-errors.c
> @@ +378,5 @@
> > +{
> > +  DBusRealError *real;
> > +  DBusString str;
> > +
> > +  _dbus_assert (name != NULL);
> 
> This doesn’t do the check:
>   _dbus_return_if_error_is_set (error);

dbus checks (as in return_if_fail) are not intended to be used in non-public APIs, and actually assert that the first character of the function name is not "_". You're meant to use asserts instead, apparently. (I didn't write this rule.)

The check is effectively done anyway, just after the error == NULL early return, because we assert that error->name and error->message are both NULL - that's a simplified version of dbus_error_is_set().
Comment 43 Philip Withnall 2015-02-03 16:14:46 UTC
(In reply to Simon McVittie from comment #41)
> (In reply to Philip Withnall from comment #32)
> > > +  g_close (address_fd, NULL);
> > 
> > I assume this means dbus-daemon gracefully ignores EPIPE?
> 
> Actually no; if we have MSG_NOSIGNAL, SIGPIPE is not ignored. However,
> dbus-daemon should not be writing anything to its stdout, other than its
> address + "\n" where requested (which we do, here). If it did, it would be
> annoying to capture the address, because you wouldn't know what was address
> and what was other rubbish...

Fair enough.

(In reply to Simon McVittie from comment #42)
> dbus checks (as in return_if_fail) are not intended to be used in non-public
> APIs, and actually assert that the first character of the function name is
> not "_". You're meant to use asserts instead, apparently. (I didn't write
> this rule.)
> 
> The check is effectively done anyway, just after the error == NULL early
> return, because we assert that error->name and error->message are both NULL
> - that's a simplified version of dbus_error_is_set().

OK!

One remaining thing: if possible, it would be great to exercise the installed-tests in gnome-continuous or via autopkgtest in Debian (or, better yet, both). Once this is merged we should look at spinning a new release and package, and talking to cwalters about whether he wants it in gnome-ci yet.
Comment 44 Simon McVittie 2015-02-03 18:25:34 UTC
Fixed in git for 1.9.8, thanks.

(In reply to Philip Withnall from comment #43)
> One remaining thing: if possible, it would be great to exercise the
> installed-tests in gnome-continuous or via autopkgtest in Debian (or, better
> yet, both). Once this is merged we should look at spinning a new release and
> package, and talking to cwalters about whether he wants it in gnome-ci yet.

Indeed. I'm looking into doing a 1.9.8 immediately.


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.