Bug 105656

Summary: Containers message filtering/policy (#101902): basic, strict policy
Product: dbus Reporter: Simon McVittie <smcv>
Component: coreAssignee: Simon McVittie <smcv>
Status: RESOLVED MOVED QA Contact: D-Bus Maintainers <dbus>
Severity: enhancement    
Priority: medium CC: alexl, bugzilla, dbus, desrt, james
Version: git masterKeywords: patch
Hardware: All   
OS: All   
URL: https://github.com/smcv/dbus/commits/filtering-105656
Whiteboard: review?
i915 platform: i915 features:
Bug Depends on:    
Bug Blocks: 101902, 105657, 105658    
Attachments: 01/39] driver: Remove references to an obsolete constant
02/39] activation: Refuse to load activatable services with invalid names
03/39] tests: Add a GAsyncReadyCallback that stores the GAsyncResult
04/39] bus_driver_get_owner_of_name: Clarify role of connection
05/39] bus_service_list_queued_owners: Don't return a DBusError
06/39] bus: Document arguments of bus_activation_activate_service
07/39] bus: Make bus_driver_send_name_owner_changed take DBusConnections
08/39] containers: Factor out connection_get_instance and make it NULL-safe
09/39] bus_registry_list_services: Take the DBusConnection that wants to know
10/39] bus_activation_list_services: Take the DBusConnection that wants to know
11/39] driver: Reorganise GetNameOwner, ListQueuedOwners for filtering
12/39] containers: Add stub functions to check against filtering
13/39] bus: If containers block a requested reply, substitute an error reply
14/39] containers test: Move teardown below all test-cases
15/39] containers test: Factor out disconnecting the unconfined manager
16/39] containers test: Factor out fixture_disconnect_observer
17/39] containers test: Record the unconfined manager connection's unique name
18/39] containers test: Maintain an array of confined connections
19/39] containers test: Monitor observer name-ownership
20/39] test: Add a function to catch up with pending messages
21/39] containers test: NameOwnerChanged is visible within a container
22/39] containers: Allow an empty array of Allow rules
23/39] containers test: Exercise invalid Allow rules
24/39] containers test: Actually add Allow rules
25/39] containers test: Optionally make second confined connection own a name
26/39] containers: Containers with an Allow policy cannot see well-known names
27/39] containers: Containers with an Allow policy cannot see most unique names
28/39] containers test: Exercise name-listing methods
29/39] containers test: Exercise visibility of a confined well-known name
30/39] containers test: Exercise visibility of observer's names
31/39] containers: Don't allow containers to send unsolicited replies
32/39] containers test: Check that unsolicited replies can't be sent
33/39] containers: Containers with an Allow policy cannot own names
34/39] containers: Containers with an Allow policy cannot activate services
35/39] containers: Containers with Allow policy can't inspect most connections
36/39] containers: Limit the messages sent by connections with an Allow policy
37/39] containers: Limit messages received by containers with Allow policy
38/39] containers test: Add a test for sending and receiving method calls
39/39] containers test: Exercise allowing/denying signals
[27½] test: Add an implementation of g_strv_contains() for GLib < 2.44
[38 v2] containers test: Add a test for sending and receiving method calls
[02+] fixup! activation: Refuse to load activatable services with invalid names
[07+] fixup! bus: Make bus_driver_send_name_owner_changed take DBusConnections
[18+] fixup! containers test: Maintain an array of confined connections
[38+] fixup! containers test: Add a test for sending and receiving method calls
[21+] fixup! containers test: NameOwnerChanged is visible within a container
[32+] fixup! containers test: Check that unsolicited replies can't be sent
[38+] fixup! containers test: Add a test for sending and receiving method calls
[21+] fixup! containers test: NameOwnerChanged is visible within a container
[28+] fixup! containers test: Exercise name-listing methods
[26+] fixup! containers: Containers with an Allow policy cannot see well-known names
[38+] fixup! containers test: Add a test for sending and receiving method calls
[38+] fixup! containers test: Add a test for sending and receiving method calls
[39 v2] containers test: Exercise allowing/denying signals

Description Simon McVittie 2018-03-21 12:37:37 UTC
+++ This bug was initially created as a clone of Bug #101902 +++

As a starting point for T7327, implement the most basic form of the Allow named argument: an empty list of things to allow.

This is planned to have the following semantics:

* Things that are unconditionally allowed are allowed
* Anything that is conditional is forbidden
Comment 1 Simon McVittie 2018-03-21 12:40:36 UTC
A sketch of acceptance criteria (these might change based on implementation experience and review, but they're close):

[ ] An AddServer call can include the Allow parameter
[ ] These *unconditionally allowed* operations are allowed for all container instances:
    [ ] Receive a method call from a peer outside, as long as it has no Unix fds
    [ ] Receive a unicast signal from a peer outside, as long as it has no Unix fds
    [ ] Send a reply or error in response to a method call from outside that expects one
    [ ] Receive NameOwnerChanged for another connection in the same instance
    [ ] Send Hello()
    [ ] Send o.fd.DBus.Peer method calls to dbus-daemon
    [ ] Send Introspect() to the dbus-daemon
    [ ] Send GetId() to the dbus-daemon
    [ ] Send AddMatch() if not eavesdropping
    [ ] Send RemoveMatch()
    [ ] Send method call or unicast signal to another connection in the same instance
    [ ] Request to own an invalid name, a unique name or o.fd.DBus (allowed, but fails with InvalidArgs)
[ ] These *conditionally allowed* operations are allowed for container instances that do not have an Allow parameter, but raise the given error for container instances that do have an Allow parameter:
    [ ] Receive a broadcast (no error, message just does not arrive)
    [ ] Send a reply or error to a spurious message serial number (AccessDenied)
    [ ] Send a second reply or error to a method call from outside (AccessDenied)
    [ ] Send a reply or error to a method call from outside that does not expect one (AccessDenied)
    [ ] Send method calls to the outside (AccessDenied)
    [ ] Send unicast signals to the outside (AccessDenied)
    [ ] Receive NameOwnerChanged for connections outside the instance (no error, signal is silently dropped)
    [ ] Request to own a well-known name (AccessDenied)
    [ ] Release a well-known name (AccessDenied - this avoids using the result as an oracle to see whether the name has another owner)
    [ ] List queued name owners of a well-known name (AccessDenied)
    [ ] List (activatable) names (no error, but only names inside the container instance + o.fd.DBus are visible)
    [ ] GetNameOwner (no error, but only names inside the container instance + o.fd.DBus are visible)
    [ ] NameHasOwner (no error, but only names inside the container instance + o.fd.DBus are visible)
    [ ] StartServiceByName (AccessDenied)
    [ ] Receive a unicast with Unix fds (AccessDenied to sender? InvalidArgs to sender?)
[ ] These *restricted* operations raise the given error for container instances with an Allow parameter, and have unspecified results for container instances without:
    [ ] Call BecomeMonitor (AccessDenied)
    [ ] Try to eavesdrop (AccessDenied)
    [ ] Call UpdateActivationEnvironment (AccessDenied)
    [ ] Stats, Verbose method calls (AccessDenied)
[ ] Unit tests
    [ ] Container instance without Allow can do all the *conditionally allowed* and *unconditionally allowed* things
    [ ] Container instance with empty Allow can do all the *unconditionally allowed* things
    [ ] Container instance with non-empty Allow cannot do any of the *conditionally allowed* things, and gets the specified error

To be designed
==============

[ ] Decide what should happen in these cases, and implement it:
    [ ] Send a broadcast (no error, but broadcast can only be received within the instance and a synthetic error is sent to monitors? or AccessDenied error?)

Out of scope
============

* Actually useful filtering (rules to allow things)
* A non-empty Allow list is not required to do anything useful (it will either be treated as empty or cause an error, at this stage)
* Absence of an Allow list is not required to allow privileged stuff like BecomeMonitor (Bug #103458)
Comment 2 Simon McVittie 2018-03-21 12:57:06 UTC
(In reply to Simon McVittie from comment #0)
> As a starting point for T7327

That should say: for Bug #101902. (Sorry, this came from a non-public task tracking system where I was planning out what's needed.)
Comment 3 Simon McVittie 2018-05-11 18:41:26 UTC
Progress: I have a version of this that passes tests, with a couple of test-cases #if 0 because they depend on "If a contained peer receives a method call or unicast signal from outside the container, then it receives SEE access to the sender's unique name" which isn't implemented yet (Bug #105658).
Comment 4 Simon McVittie 2018-05-11 18:42:32 UTC
(In reply to Simon McVittie from comment #3)
> Progress: I have a version of this that passes tests

It's going to need some rebasing before it's reviewable, though, particularly the tests. Currently at:

 26 files changed, 3919 insertions(+), 273 deletions(-)

which is about 75% tests.
Comment 5 Simon McVittie 2018-06-01 19:00:43 UTC
Created attachment 139908 [details] [review]
01/39] driver: Remove references to an obsolete constant
Comment 6 Simon McVittie 2018-06-01 19:01:03 UTC
Created attachment 139909 [details] [review]
02/39] activation: Refuse to load activatable services with  invalid names

We aren't going to allow activating them anyway, so there's no point
in having them in memory.

(A secondary justification is that this makes my Containers1 work
more straightforward, by making sure I don't have to handle the case
where unique names, malformed names or org.freedesktop.DBus can be
included in the entries hash table.)
Comment 7 Simon McVittie 2018-06-01 19:01:25 UTC
Created attachment 139911 [details] [review]
03/39] tests: Add a GAsyncReadyCallback that stores the  GAsyncResult

It seems I eventually introduce this into every project where I've
added GLib-based unit tests. Today it's dbus' turn.
Comment 8 Simon McVittie 2018-06-01 19:01:45 UTC
Created attachment 139912 [details] [review]
04/39] bus_driver_get_owner_of_name: Clarify role of  connection

This connection is the one looking at the name, as opposed to the
one that owns the name (if any).
Comment 9 Simon McVittie 2018-06-01 19:02:04 UTC
Created attachment 139914 [details] [review]
05/39] bus_service_list_queued_owners: Don't return a  DBusError

This makes it clearer that the only possible error is out-of-memory,
so its use in ListQueuedOwners() is not leaking information to callers
that might not be allowed to know the difference between "doesn't exist"
and "exists but you are not allowed to know that".
Comment 10 Simon McVittie 2018-06-01 19:02:22 UTC
Created attachment 139915 [details] [review]
06/39] bus: Document arguments of  bus_activation_activate_service

It isn't completely obvious that connection is allowed to be NULL here.
Comment 11 Simon McVittie 2018-06-01 19:02:46 UTC
Created attachment 139916 [details] [review]
07/39] bus: Make bus_driver_send_name_owner_changed take  DBusConnections

All callers had access to the DBusConnection that is the old owner,
and the DBusConnection that is the new owner. Pass those to the
function instead of their names. This gives the function a bit more
contextual information, which will be important when we filter
NameOwnerChanged messages entering containers according to whether the
container is allowed to see the connection.

While I'm breaking its API anyway, rename the function to match the
signal that it emits, which was renamed in early 2005.
Comment 12 Simon McVittie 2018-06-01 19:03:06 UTC
Created attachment 139917 [details] [review]
08/39] containers: Factor out connection_get_instance and make  it NULL-safe

When I introduce per-container message filtering, it'll be useful to
be able to get the instance for a connection without worrying about
whether that connection is NULL (representing the dbus-daemon itself,
or an activatable service that has not yet been activated).

Also make it robust against Containers having not been initialized,
for completeness.
Comment 13 Simon McVittie 2018-06-01 19:03:27 UTC
Created attachment 139919 [details] [review]
09/39] bus_registry_list_services: Take the DBusConnection  that wants to know

This will be necessary when we give connections from containers a
different answer that only lists a subset of the names.
Comment 14 Simon McVittie 2018-06-01 19:03:46 UTC
Created attachment 139920 [details] [review]
10/39] bus_activation_list_services: Take the DBusConnection  that wants to know

This will be necessary when we give connections from containers a
different answer that only lists a subset of the names.
Comment 15 Simon McVittie 2018-06-01 19:04:10 UTC
Created attachment 139921 [details] [review]
11/39] driver: Reorganise GetNameOwner, ListQueuedOwners for  filtering

This converts them to a mostly early-return pattern (via a goto because
some cleanup is needed).

If the desired name is DBUS_SERVICE_DBUS, we can just early-return
without doing the other work.

Similarly, if the desired name is not syntactically valid, we can
early-return without even asking the registry whether someone owns it,
because nobody can own an invalid name.

Add a specific code path for reporting that the name doesn't exist, so
that when we filter, all code paths that should be indistinguishable
(can't see because it doesn't exist, or because policy says we can't
see it) will be using the same code.
Comment 16 Simon McVittie 2018-06-01 19:04:41 UTC
Created attachment 139923 [details] [review]
12/39] containers: Add stub functions to check against  filtering

---

This could maybe be broken down further, but life's too short.
Comment 17 Simon McVittie 2018-06-01 19:05:40 UTC
Created attachment 139925 [details] [review]
13/39] bus: If containers block a requested reply, substitute  an error reply

If some connection in a container has legitimately had one of its
methods called by another connection (either unconfined or in a
different container), but the reply is unsuitably for delivery to
the caller (perhaps because it contains fds), we don't want the
caller to be left waiting. Add a way for the security policy to
request that we send an error reply to the original method call, as
well as sending an error reply to the undesirable reply.
Comment 18 Simon McVittie 2018-06-01 19:05:53 UTC
Created attachment 139926 [details] [review]
14/39] containers test: Move teardown below all test-cases
Comment 19 Simon McVittie 2018-06-01 19:06:10 UTC
Created attachment 139927 [details] [review]
15/39] containers test: Factor out disconnecting the  unconfined manager

As this test's coverage expands, this function will have to do more
(clear up name watches, filters, etc.) so it'll be helpful to keep it
all in one place.
Comment 20 Simon McVittie 2018-06-01 19:06:28 UTC
Created attachment 139928 [details] [review]
16/39] containers test: Factor out fixture_disconnect_observer
Comment 21 Simon McVittie 2018-06-01 19:06:43 UTC
Created attachment 139929 [details] [review]
17/39] containers test: Record the unconfined manager  connection's unique name

This is a bit more convenient than fetching it as-needed.
Comment 22 Simon McVittie 2018-06-01 19:06:58 UTC
Created attachment 139930 [details] [review]
18/39] containers test: Maintain an array of confined  connections
Comment 23 Simon McVittie 2018-06-01 19:08:08 UTC
Created attachment 139931 [details] [review]
19/39] containers test: Monitor observer name-ownership

We can use this after disconnecting the observer, as a way to work out
when a confined connection that might not be allowed to see the
observer would have seen NameOwnerChanged for the observer if it
was ever going to: if the unconfined connection has seen the observer
disappear, and the confined connection has seen a message that the
unconfined connection only sent after it saw the observer disappear,
then causal ordering means the confined connection would also have
seen NameOwnerChanged (if it was going to).

---

Reading that paragraph again, I'm having trouble understanding it myself, but I promise this is a useful function :-)

It will become useful after 30/39.
Comment 24 Simon McVittie 2018-06-01 19:08:34 UTC
Created attachment 139932 [details] [review]
20/39] test: Add a function to catch up with pending messages

This is very loosely based on test infrastructure that used to exist in
Telepathy. It arranges for all prior messages on both the connections to
have been processed, by making use of total ordering: if caller sends
method call M1 to callee, and callee sends back a reply M2, then all
messages received by callee prior to M1, received by caller prior to
M2, sent by callee prior to M2 or sent by caller prior to M1 are also
guaranteed to have been processed.
Comment 25 Simon McVittie 2018-06-01 19:09:08 UTC
Created attachment 139933 [details] [review]
21/39] containers test: NameOwnerChanged is visible within a  container

At the moment this is trivially true because we don't filter messages
at all, but this test-case adds infrastructure for checking less
trivial situations under different rulesets.
Comment 26 Simon McVittie 2018-06-01 19:09:46 UTC
Created attachment 139934 [details] [review]
22/39] containers: Allow an empty array of Allow rules

This is the least possible we can do: the existence of an Allow array
sets a flag that says "this connection applies policies", but we only
allow one policy, namely "reject almost everything".
Comment 27 Simon McVittie 2018-06-01 19:10:16 UTC
Created attachment 139935 [details] [review]
23/39] containers test: Exercise invalid Allow rules

We don't actually accept any Allow rules yet, so the single Allow rule
that we try to add is trivially invalid; but when we start adding
possible Allow rules, it will become more useful to verify that
obviously-invalid rules remain invalid.
Comment 28 Simon McVittie 2018-06-01 19:11:24 UTC
Created attachment 139936 [details] [review]
24/39] containers test: Actually add Allow rules

---

... or at least we would if the array was ever non-empty, so this is partially dead code right now. It will become directly useful as soon as I implement at least one rule that we can add...
Comment 29 Simon McVittie 2018-06-01 19:11:44 UTC
Created attachment 139937 [details] [review]
25/39] containers test: Optionally make second confined  connection own a name

This also adds convenience infrastructure for owning names.
Comment 30 Simon McVittie 2018-06-01 19:12:10 UTC
Created attachment 139938 [details] [review]
26/39] containers: Containers with an Allow policy cannot see  well-known names

There is one exception: they can see the dbus-daemon itself.
Comment 31 Simon McVittie 2018-06-01 19:12:29 UTC
Created attachment 139939 [details] [review]
27/39] containers: Containers with an Allow policy cannot see  most unique names

We unconditionally allow connections in the same container to see each
other, and as a trivial case, allow a connection to see itself.
Comment 32 Simon McVittie 2018-06-01 19:12:50 UTC
Created attachment 139940 [details] [review]
28/39] containers test: Exercise name-listing methods

These are currently controlled by the hard-coded baseline policy
(containers with no Allow list can see all names, containers with
an empty Allow list cannot see most names) but will eventually have
finer granularity via non-empty Allow lists.
Comment 33 Simon McVittie 2018-06-01 19:13:12 UTC
Created attachment 139941 [details] [review]
29/39] containers test: Exercise visibility of a confined  well-known name

We can currently only test this for the case where we have no
policy. When we add Allow rules that allow owning a name, we can
test that the name is also correctly visible.
Comment 34 Simon McVittie 2018-06-01 19:13:43 UTC
Created attachment 139942 [details] [review]
30/39] containers test: Exercise visibility of observer's  names

This tests whether the contained connection can see the unique and
well-known names of a connection that never communicated with it.
Comment 35 Simon McVittie 2018-06-01 19:14:04 UTC
Created attachment 139943 [details] [review]
31/39] containers: Don't allow containers to send unsolicited  replies

Containers are new functionality, so we don't need to preserve
historical warts, like the fact that the behaviour for unsolicited
replies is (inexplicably) sysadmin-configurable.
Comment 36 Simon McVittie 2018-06-01 19:14:31 UTC
Created attachment 139944 [details] [review]
32/39] containers test: Check that unsolicited replies can't  be sent

To do this, we need to send messages that contain a special message
body, since there is no straightforward way to tell which message
is which otherwise.

Strictly speaking, this only needs the filter on the unconfined
connection (that's where we're sending the unsolicited replies),
but adding the same filter to the unconfined observer connection
and to each confined connection will make life easier for subsequent
tests.
Comment 37 Simon McVittie 2018-06-01 19:14:51 UTC
Created attachment 139945 [details] [review]
33/39] containers: Containers with an Allow policy cannot own  names

In future we will offer a way to allow them to own names, but this is
not yet implemented.
Comment 38 Simon McVittie 2018-06-01 19:15:05 UTC
Created attachment 139946 [details] [review]
34/39] containers: Containers with an Allow policy cannot  activate services
Comment 39 Simon McVittie 2018-06-01 19:15:27 UTC
Created attachment 139947 [details] [review]
35/39] containers: Containers with Allow policy can't inspect  most connections

By "inspect" I mean methods like GetConnectionCredentials.

Again, we make an exception for connections that share a container.
Comment 40 Simon McVittie 2018-06-01 19:15:54 UTC
Created attachment 139948 [details] [review]
36/39] containers: Limit the messages sent by connections with  an Allow policy

The baseline rules for connections in containers with a policy are
as follows:

* Can send legitimate replies that don't contain Unix fds
* Can't send Unix fds (as an easy way to mitigate the complexity
  and low arbitrary limits of Unix fd passing, which carries a risk
  of exploitable bugs)
* Can send messages to the dbus-daemon that don't contain Unix fds
* Can send messages to connections in the same container (including,
  as a trivial case, themselves)
* Can't send anything not explicitly allowed
Comment 41 Simon McVittie 2018-06-01 19:16:18 UTC
Created attachment 139949 [details] [review]
37/39] containers: Limit messages received by containers with  Allow policy

The baseline for containers with an Allow policy is as follows:

* Containers may not receive Unix fds, as an easy way to mitigate the
  complexity and risk of bugs that result from fd-passing
* Unicast messages (that were allowed to be sent) are always allowed
* NameOwnerChanged broadcast messages from the dbus-daemon are
  checked elsewhere and so are always allowed here
* Connections within one container may broadcast among themselves
* Other broadcasts are not allowed

Future enhancements will add Allow rules that allow receiving Unix
fds and allow receiving broadcasts.
Comment 42 Simon McVittie 2018-06-01 19:16:38 UTC
Created attachment 139950 [details] [review]
38/39] containers test: Add a test for sending and receiving  method calls

In addition to the messages themselves, this exercises name ownership.
Comment 43 Simon McVittie 2018-06-01 19:17:40 UTC
Created attachment 139951 [details] [review]
39/39] containers test: Exercise allowing/denying signals
Comment 44 Simon McVittie 2018-06-01 19:38:39 UTC
Comment on attachment 139940 [details] [review]
28/39] containers test: Exercise name-listing methods

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

Travis-CI tells me we will need a compatibility definition of g_strv_contains() for ye olde GLib:

../../test/containers.c:546:3: error: nested extern declaration of ‘g_strv_contains’ [-Werror=nested-externs]
Comment 45 Simon McVittie 2018-06-04 12:37:17 UTC
Created attachment 140008 [details] [review]
[27½] test: Add an implementation of g_strv_contains() for  GLib < 2.44

We still use Ubuntu 14.04 on Travis-CI, and that doesn't have g_strv_contains().
Comment 46 Simon McVittie 2018-06-04 12:43:14 UTC
Created attachment 140009 [details] [review]
[38 v2] containers test: Add a test for sending and receiving  method calls

In addition to the messages themselves, this exercises name ownership.   

---

Don't assume that GetMachineId() will succeed, because minimal autobuilder environments that have neither dbus nor systemd installed will not always have a machine ID that we can use.

Where we are testing both GetMachineId() and Ping(), use METHOD_ALLOWS_ACCESS instead of METHOD_SUCCEEDS for GetMachineId() - this will allow either success, or any failure other than ACCESS_DENIED. In particular FILE_NOT_FOUND is OK, and that's the error raised by GetMachineId() if /etc/machine-id and ${localstatedir}/dbus/machine-id don't exist.

Where we are only testing GetMachineId(), use Ping() instead, because it's simpler: we can assume it will always succeed.
Comment 47 Philip Withnall 2018-06-04 16:22:44 UTC
Comment on attachment 139908 [details] [review]
01/39] driver: Remove references to an obsolete constant

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

r+
Comment 48 Philip Withnall 2018-06-11 11:17:30 UTC
Comment on attachment 139909 [details] [review]
02/39] activation: Refuse to load activatable services with  invalid names

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

Generally r+, but with a couple of suggestions which you might want to deal with or ignore.

::: bus/activation.c
@@ +325,5 @@
> +      dbus_set_error (error, DBUS_ERROR_INVALID_ARGS,
> +                      "Bus name \"%s\" cannot be activatable because it "
> +                      "is reserved for the message bus", name);
> +      goto out;
> +    }

Would it make sense to factor out these three checks into a new dbus_validate_activatable_bus_name() or something?

@@ +2790,5 @@
>    return ret;
>  }
>  
> +void
> +bus_activation_check_services (BusActivation *self)

Nitpick: A style I’ve been using recently in my code is to put a summary comment above each unit test function, briefly describing what the test does and what it’s trying to test. I don’t know if that’s something you want to adopt too. I find it’s helped when I go back to tests later.

::: test/data/invalid-service-files/org.freedesktop.DBus.TestSuiteNotValidName.service
@@ +1,2 @@
> +[D-BUS Service]
> +Name=?!

What about a test for an empty `Name=`?
Comment 49 Philip Withnall 2018-06-11 11:18:18 UTC
Comment on attachment 139911 [details] [review]
03/39] tests: Add a GAsyncReadyCallback that stores the  GAsyncResult

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

Trivially r+
Comment 50 Philip Withnall 2018-06-11 11:19:06 UTC
Comment on attachment 139912 [details] [review]
04/39] bus_driver_get_owner_of_name: Clarify role of  connection

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

r+
Comment 51 Philip Withnall 2018-06-11 11:19:47 UTC
Comment on attachment 139914 [details] [review]
05/39] bus_service_list_queued_owners: Don't return a  DBusError

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

r+
Comment 52 Philip Withnall 2018-06-11 11:21:45 UTC
Comment on attachment 139915 [details] [review]
06/39] bus: Document arguments of  bus_activation_activate_service

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

r+
Comment 53 Philip Withnall 2018-06-11 11:25:18 UTC
Comment on attachment 139916 [details] [review]
07/39] bus: Make bus_driver_send_name_owner_changed take  DBusConnections

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

r-

::: bus/driver.c
@@ +252,5 @@
>  
>    _DBUS_ASSERT_ERROR_IS_CLEAR (error);
>  
> +  if (old_owner != NULL)
> +    old_owner_name = bus_connection_get_name (old_owner);

Looks like it’s possible for bus_connection_get_name() to return NULL if the service is inactive (see bus_connection_is_active()), in which case you’ll have `old_owner_name == NULL` rather than `old_owner_name == ""`.
Comment 54 Philip Withnall 2018-06-11 11:27:50 UTC
Comment on attachment 139917 [details] [review]
08/39] containers: Factor out connection_get_instance and make  it NULL-safe

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

r+
Comment 55 Philip Withnall 2018-06-11 11:29:18 UTC
Comment on attachment 139919 [details] [review]
09/39] bus_registry_list_services: Take the DBusConnection  that wants to know

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

r+
Comment 56 Philip Withnall 2018-06-11 11:29:46 UTC
Comment on attachment 139920 [details] [review]
10/39] bus_activation_list_services: Take the DBusConnection  that wants to know

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

r+
Comment 57 Simon McVittie 2018-06-20 11:50:20 UTC
(In reply to Philip Withnall from comment #48)
> Comment on attachment 139909 [details] [review]
> 02/39] activation: Refuse to load activatable services with  invalid names

> ::: bus/activation.c
> @@ +325,5 @@
> > +      dbus_set_error (error, DBUS_ERROR_INVALID_ARGS,
> > +                      "Bus name \"%s\" cannot be activatable because it "
> > +                      "is reserved for the message bus", name);
> > +      goto out;
> > +    }
> 
> Would it make sense to factor out these three checks into a new
> dbus_validate_activatable_bus_name() or something?

IMO not unless/until we have another user for it. Are you thinking public API or internal?

In principle I suppose we could have a variant of dbus_validate_bus_name() called dbus_validate_well_known_name(), because that's what this is really - org.freedesktop.DBus is special, and is neither well-known nor unique (it behaves more like a unique name but has the syntax of a well-known name).

Prior art: GDBus has g_dbus_is_unique_name() and g_dbus_is_name(), but expects you to know that every bus name that is valid, not unique and not org.freedesktop.DBus is well-known. dbus-python has an internal function dbus_py_validate_bus_name(const char *, dbus_bool_t may_be_unique, dbus_bool_t may_be_not_unique) but doesn't special-case org.freedesktop.DBus there. telepathy-glib is perhaps the most pedantically correct, and has tp_dbus_check_valid_bus_name(const char *, TpDBusNameType, GError **) where TpDBusNameType is a bitfield { UNIQUE, WELL_KNOWN, DBUS_DAEMON } plus convenience aliases NOT_DBUS_DAEMON and ANY.

If we want this as public API then I think that should be a separate feature request.

> Nitpick: A style I’ve been using recently in my code is to put a summary
> comment above each unit test function

Good idea, added in v2

> What about a test for an empty `Name=`?

Good idea, added in v2
Comment 58 Simon McVittie 2018-06-20 11:56:31 UTC
(In reply to Philip Withnall from comment #53)
> Comment on attachment 139916 [details] [review]
> 07/39] bus: Make bus_driver_send_name_owner_changed take  DBusConnections

> > +  if (old_owner != NULL)
> > +    old_owner_name = bus_connection_get_name (old_owner);
> 
> Looks like it’s possible for bus_connection_get_name() to return NULL if the
> service is inactive (see bus_connection_is_active()), in which case you’ll
> have `old_owner_name == NULL` rather than `old_owner_name == ""`.

Inactive connections can't own names. I've added assertions for v2.

When a connection becomes active, it is given its unique name in its data structure before we announce that it has received it and emit NameOwnerChanged.

The only method that inactive connections are allowed to call is o.fd.DBus.Hello (there is a hard-coded check), so they have no opportunity to receive a well-known name, which can only be done by calling o.fd.DBus.RequestName.

When a connection disconnects, it never actually becomes inactive, it just goes away.

Monitors lose ownership of their unique name, but it stays in their data structure and the monitor stays active forever.
Comment 59 Philip Withnall 2018-06-20 15:12:58 UTC
(In reply to Simon McVittie from comment #57)
> (In reply to Philip Withnall from comment #48)
> > Would it make sense to factor out these three checks into a new
> > dbus_validate_activatable_bus_name() or something?
> 
> IMO not unless/until we have another user for it. Are you thinking public
> API or internal?

I was thinking internal API.
Comment 60 Philip Withnall 2018-06-20 15:19:32 UTC
Comment on attachment 139921 [details] [review]
11/39] driver: Reorganise GetNameOwner, ListQueuedOwners for  filtering

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

r+
Comment 61 Philip Withnall 2018-06-20 15:36:25 UTC
Comment on attachment 139923 [details] [review]
12/39] containers: Add stub functions to check against  filtering

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

r+
Comment 62 Philip Withnall 2018-06-20 15:43:05 UTC
Comment on attachment 139925 [details] [review]
13/39] bus: If containers block a requested reply, substitute  an error reply

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

r+ apart from the commit message:

Nitpick: s/unsuitably/unsuitable/ in the commit message.
Comment 63 Philip Withnall 2018-06-20 15:43:27 UTC
Comment on attachment 139926 [details] [review]
14/39] containers test: Move teardown below all test-cases

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

Trivially r+
Comment 64 Philip Withnall 2018-06-20 15:45:39 UTC
Comment on attachment 139927 [details] [review]
15/39] containers test: Factor out disconnecting the  unconfined manager

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

r+
Comment 65 Philip Withnall 2018-06-20 15:46:13 UTC
Comment on attachment 139928 [details] [review]
16/39] containers test: Factor out fixture_disconnect_observer

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

r+
Comment 66 Philip Withnall 2018-06-20 15:46:56 UTC
Comment on attachment 139929 [details] [review]
17/39] containers test: Record the unconfined manager  connection's unique name

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

r+
Comment 67 Philip Withnall 2018-06-20 15:51:47 UTC
Comment on attachment 139930 [details] [review]
18/39] containers test: Maintain an array of confined  connections

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

r-, some comments.

::: test/containers.c
@@ +234,5 @@
> +      else
> +        g_assert_no_error (error);
> +    }
> +
> +  g_clear_object (&f->confined_conns[i]);

Would it make sense to clear the corresponding f->confined_unique_names[i] here too?

@@ +546,5 @@
>    g_assert_null (tuple);
>  
>    g_test_message ("Inspecting connection container info");
> +  f->confined_unique_names[0] = g_strdup (
> +      g_dbus_connection_get_unique_name (f->confined_conns[0]));

Does f->confined_unique_names[0] need to be set here again, after it’s set in fixture_connect_confined()?

@@ +1625,4 @@
>  teardown (Fixture *f,
>      gconstpointer context G_GNUC_UNUSED)
>  {
> +  guint i;

Nitpick: I’d use gsize for array indexing, as a general rule. In this case, it’s completely irrelevant though.
Comment 68 Philip Withnall 2018-06-20 16:01:50 UTC
Comment on attachment 139931 [details] [review]
19/39] containers test: Monitor observer name-ownership

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

r+
Comment 69 Philip Withnall 2018-06-20 16:05:50 UTC
Comment on attachment 139932 [details] [review]
20/39] test: Add a function to catch up with pending messages

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

r+
Comment 70 Simon McVittie 2018-06-21 16:52:41 UTC
(In reply to Philip Withnall from comment #67)
> Comment on attachment 139930 [details] [review]
> 18/39] containers test: Maintain an array of confined  connections

> > +  g_clear_object (&f->confined_conns[i]);
> 
> Would it make sense to clear the corresponding f->confined_unique_names[i]
> here too?

No: we might still want to remember which unique name corresponded to which confined connection, so that we can make assertions about the NameOwnerChanged signals that result from the confined connection going away (and in fact I think a later patch does just that).

> > +  f->confined_unique_names[0] = g_strdup (
> > +      g_dbus_connection_get_unique_name (f->confined_conns[0]));
> 
> Does f->confined_unique_names[0] need to be set here again, after it’s set
> in fixture_connect_confined()?

No, and it's a memory leak. Good catch!

> > +  guint i;
> 
> Nitpick: I’d use gsize for array indexing, as a general rule. In this case,
> it’s completely irrelevant though.

Fine, it's gsize now (and so is the parameter to fixture_connect_confined() and fixture_disconnect_confined()).
Comment 71 Simon McVittie 2018-06-21 16:54:04 UTC
(In reply to Philip Withnall from comment #59)
> (In reply to Simon McVittie from comment #57)
> > IMO not unless/until we have another user for it. Are you thinking public
> > API or internal?
> 
> I was thinking internal API.

Then my answer is no, we're not going to need it, unless/until a second user for that function proves me wrong.
Comment 72 Simon McVittie 2018-06-21 18:06:24 UTC
> 01/39] driver: Remove references to an obsolete constant
> 03/39] tests: Add a GAsyncReadyCallback that stores the  GAsyncResult
> 04/39] bus_driver_get_owner_of_name: Clarify role of  connection
> 05/39] bus_service_list_queued_owners: Don't return a  DBusError
> 06/39] bus: Document arguments of  bus_activation_activate_service
> 08/39] containers: Factor out connection_get_instance and make  it NULL-safe
> 14/39] containers test: Move teardown below all test-cases
> 15/39] containers test: Factor out disconnecting the  unconfined manager
> 16/39] containers test: Factor out fixture_disconnect_observer
> 17/39] containers test: Record the unconfined manager  connection's unique
> name

I rebased the branch to sort these reviewed/uncontroversial patches to the beginning, and pushed them to the `try` branch on my github. I'll push them to master, assuming CI succeeds.
Comment 73 Simon McVittie 2018-06-21 18:08:07 UTC
Created attachment 140269 [details] [review]
[02+] fixup! activation: Refuse to load activatable services  with invalid names

* Add TestSuiteNoName test case
* Add more comments

---

To be squashed in the obvious way into [02].
Comment 74 Simon McVittie 2018-06-21 18:08:53 UTC
Created attachment 140270 [details] [review]
[07+] fixup! bus: Make bus_driver_send_name_owner_changed  take DBusConnections

Justify why the names can't be NULL, and add assertions
Comment 75 Simon McVittie 2018-06-21 18:09:38 UTC
Created attachment 140271 [details] [review]
[18+] fixup! containers test: Maintain an array of confined  connections

Remove duplicate set of confined_unique_names[0].

Use gsize to index the array.
Comment 76 Simon McVittie 2018-06-21 18:10:33 UTC
Created attachment 140272 [details] [review]
[38+] fixup! containers test: Add a test for sending and  receiving method calls

Don't test EnableVerbose() or GetStats() if that functionality was
not compiled in. For dbus-daemon (only), the check for whether we
know the method comes before the filtering.
Comment 77 Philip Withnall 2018-06-21 21:34:58 UTC
r+ on the fixups 02+, 07+, 18+

I can’t review the fixup for 38 yet, since I haven’t yet reviewed 38 itself.
Comment 78 Philip Withnall 2018-06-21 22:09:20 UTC
Comment on attachment 139933 [details] [review]
21/39] containers test: NameOwnerChanged is visible within a  container

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

Solid r+

::: test/containers.c
@@ +2054,4 @@
>    gchar *runtime_dbus_dir;
>    gchar *runtime_containers_dir;
>    gchar *runtime_services_dir;
> +  guint i;

Nitpick: gsize here (but I really don’t expect a fixup for this patch, since this is the only issue I’ve spotted)
Comment 79 Philip Withnall 2018-06-21 22:14:07 UTC
Comment on attachment 139934 [details] [review]
22/39] containers: Allow an empty array of Allow rules

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

r+
Comment 80 Philip Withnall 2018-06-21 22:16:46 UTC
Comment on attachment 139935 [details] [review]
23/39] containers test: Exercise invalid Allow rules

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

r+
Comment 81 Philip Withnall 2018-06-21 22:19:09 UTC
Comment on attachment 139936 [details] [review]
24/39] containers test: Actually add Allow rules

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

r+
Comment 82 Philip Withnall 2018-06-21 22:21:22 UTC
Comment on attachment 139937 [details] [review]
25/39] containers test: Optionally make second confined  connection own a name

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

r+
Comment 83 Philip Withnall 2018-06-21 22:22:48 UTC
Comment on attachment 139938 [details] [review]
26/39] containers: Containers with an Allow policy cannot see  well-known names

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

r+
Comment 84 Philip Withnall 2018-06-21 22:26:04 UTC
Comment on attachment 139938 [details] [review]
26/39] containers: Containers with an Allow policy cannot see  well-known names

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

Actually, having looked at the following patch: Does it make sense for containers to be able to see their own well-known name (if they own one)?
Comment 85 Philip Withnall 2018-06-21 22:31:25 UTC
Comment on attachment 139939 [details] [review]
27/39] containers: Containers with an Allow policy cannot see  most unique names

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

r+
Comment 86 Philip Withnall 2018-06-21 22:32:34 UTC
Comment on attachment 140008 [details] [review]
[27½] test: Add an implementation of g_strv_contains() for  GLib < 2.44

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

r+
Comment 87 Philip Withnall 2018-06-21 22:52:34 UTC
Comment on attachment 139940 [details] [review]
28/39] containers test: Exercise name-listing methods

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

r+, although I have a question about one of the comments.

::: test/containers.c
@@ +411,5 @@
> + * Helper for Allow tests: Assert that GetNameOwner(), NameHasOwner()
> + * and the given result of ListNames() agree.
> + *
> + * @names is really a (const gchar * const *) but it's passed via a
> + * gconstpointer to avoid a lot of very ugly casts.

Sad, but reasonable, times.

@@ +720,5 @@
> +  "com.example.SendDeniedByNonexistentAppArmorLabel",
> +  "com.example.SystemdActivatable1",
> +  "com.example.SystemdActivatable2",
> +  "com.example.SystemdActivatable3",
> +  /* For some reason this counts as activatable too */

‘For some reason’ does not fill me with confidence. Presumably this is just some historical quirk which we’re stuck with?
Comment 88 Philip Withnall 2018-06-21 22:55:13 UTC
Comment on attachment 139941 [details] [review]
29/39] containers test: Exercise visibility of a confined  well-known name

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

r+
Comment 89 Philip Withnall 2018-06-21 23:01:00 UTC
Comment on attachment 139942 [details] [review]
30/39] containers test: Exercise visibility of observer's  names

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

r+, nice
Comment 90 Simon McVittie 2018-06-25 09:54:28 UTC
(In reply to Philip Withnall from comment #87)
> Comment on attachment 139940 [details] [review]
> 28/39] containers test: Exercise name-listing methods
> > +  /* For some reason this counts as activatable too */
> 
> ‘For some reason’ does not fill me with confidence. Presumably this is just
> some historical quirk which we’re stuck with?

It's a fact about ListActivatableNames(), not a fact about filtering. I'm not going to change what ListActivatableNames() does for unfiltered connections on this branch.

ListActivatableNames() listing org.freedesktop.DBus appears to be deliberate (someone had to write a couple of lines of extra code to make it happen) but I don't know why. I'll open a separate bug for clarification.
Comment 91 Philip Withnall 2018-06-25 14:50:03 UTC
(In reply to Simon McVittie from comment #90)
> It's a fact about ListActivatableNames(), not a fact about filtering. I'm
> not going to change what ListActivatableNames() does for unfiltered
> connections on this branch.
> 
> ListActivatableNames() listing org.freedesktop.DBus appears to be deliberate
> (someone had to write a couple of lines of extra code to make it happen) but
> I don't know why. I'll open a separate bug for clarification.

FTR: https://bugs.freedesktop.org/show_bug.cgi?id=107024
Comment 92 Philip Withnall 2018-06-25 14:52:08 UTC
Comment on attachment 139943 [details] [review]
31/39] containers: Don't allow containers to send unsolicited  replies

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

r+
Comment 93 Philip Withnall 2018-06-25 15:08:08 UTC
Comment on attachment 139944 [details] [review]
32/39] containers test: Check that unsolicited replies can't  be sent

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

r-, some leaks

::: test/containers.c
@@ +2797,5 @@
> +      f->confined_unique_names[0], "/", DBUS_INTERFACE_PEER, "Ping");
> +  g_dbus_message_set_flags (message_without_reply,
> +                            G_DBUS_MESSAGE_FLAGS_NO_REPLY_EXPECTED);
> +  message_with_reply = g_dbus_message_new_method_call (
> +      f->confined_unique_names[0], "/", DBUS_INTERFACE_PEER, "Ping");

message_with_reply and message_without_reply are leaked.

@@ +2822,5 @@
> +  while (result == NULL)
> +    g_main_context_iteration (NULL, TRUE);
> +
> +  reply_message = g_dbus_connection_send_message_with_reply_finish (
> +      f->unconfined_conn, result, &f->error);

result and reply_message are leaked.

@@ +2832,5 @@
> +   * won't arrive at the unconfined connection and trip the check
> +   * in allow_tests_message_filter(). */
> +  for (use_error_reply = 0; use_error_reply < 2; use_error_reply++)
> +    {
> +      guint32 reply_serials[] =

Nitpick: Could be const.

@@ +2848,5 @@
> +        {
> +          GDBusMessage *unsolicited_reply;
> +          guint32 reply_serial = reply_serials[i];
> +
> +          unsolicited_reply = g_dbus_message_new ();

Is unsolicited_reply leaked at the end of each iteration?
Comment 94 Philip Withnall 2018-06-25 15:08:46 UTC
Comment on attachment 139945 [details] [review]
33/39] containers: Containers with an Allow policy cannot own  names

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

r+
Comment 95 Philip Withnall 2018-06-25 15:11:34 UTC
Comment on attachment 139946 [details] [review]
34/39] containers: Containers with an Allow policy cannot  activate services

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

r+
Comment 96 Philip Withnall 2018-06-25 15:13:25 UTC
Comment on attachment 139946 [details] [review]
34/39] containers: Containers with an Allow policy cannot  activate services

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

Hmm, although if o.fd.DBus is activatable (wrt ListActivatableNames), perhaps activating that should always be allowed? I’m not sure what that would mean, but it may be more consistent with the fact that o.fd.DBus is listed in ListActivatableNames than denying activating it.
Comment 97 Philip Withnall 2018-06-25 15:14:45 UTC
Comment on attachment 139947 [details] [review]
35/39] containers: Containers with Allow policy can't inspect  most connections

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

r+
Comment 98 Philip Withnall 2018-06-25 15:23:04 UTC
Comment on attachment 139948 [details] [review]
36/39] containers: Limit the messages sent by connections with  an Allow policy

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

r+
Comment 99 Philip Withnall 2018-06-25 15:30:11 UTC
Comment on attachment 139949 [details] [review]
37/39] containers: Limit messages received by containers with  Allow policy

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

r+
Comment 100 Philip Withnall 2018-06-26 09:31:57 UTC
Comment on attachment 140009 [details] [review]
[38 v2] containers test: Add a test for sending and receiving  method calls

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

r+ with one nitpick. I haven’t reviewed the arrays of test cases in detail, but I assume they are reasonable. Have you been looking at the code coverage of the new code while writing all these tests?

::: test/containers.c
@@ +644,5 @@
> +      reply = g_dbus_message_new_method_error (message,
> +          DBUS_ERROR_UNKNOWN_METHOD, "No such method");
> +    }
> +   else if (g_strcmp0 (g_dbus_message_get_interface (message),
> +                      DBUS_INTERFACE_PROPERTIES) == 0 &&

Nitpick: Looks like an indentation problem here, but it might just be Bugzilla.
Comment 101 Philip Withnall 2018-06-26 09:32:28 UTC
Comment on attachment 140272 [details] [review]
[38+] fixup! containers test: Add a test for sending and  receiving method calls

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

r+
Comment 102 Philip Withnall 2018-06-26 09:51:27 UTC
Comment on attachment 139951 [details] [review]
39/39] containers test: Exercise allowing/denying signals

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

r+. Just wondering (not related to anything I saw/didn’t see in this patch): have you run all the new tests through valgrind?
Comment 103 Simon McVittie 2018-07-03 17:49:53 UTC
(In reply to Philip Withnall from comment #84)
> 26/39] containers: Containers with an Allow policy cannot see  well-known
> names
> 
> Actually, having looked at the following patch: Does it make sense for
> containers to be able to see their own well-known name (if they own one)?

There is no answer to this question, because they can't own well-known names. The ability to own well-known names is a later feature (which I now realise I haven't yet opened).

When it's implemented, my intention is for a rule "may OWN com.example.Foo" to imply at least "may SEE com.example.Foo" (even if someone else currently owns com.example.Foo), but until that's possible, there's no need to have a special case for it.
Comment 104 Simon McVittie 2018-07-03 18:17:09 UTC
(In reply to Philip Withnall from comment #93)
> 32/39] containers test: Check that unsolicited replies can't  be sent
>
> Is unsolicited_reply leaked at the end of each iteration?

Yes. Fixing that and all the other leaks you spotted.
Comment 105 Simon McVittie 2018-07-03 18:19:41 UTC
(In reply to Philip Withnall from comment #100)
> [38 v2] containers test: Add a test for sending and receiving  method calls
>
> Have you been looking at the code coverage
> of the new code while writing all these tests?

No, only at the specification I wrote. You're right that I should run this branch through a coverage check though.

> Nitpick: Looks like an indentation problem here

Correct, the "else if" is off by 1 space.
Comment 106 Simon McVittie 2018-07-03 18:20:46 UTC
(In reply to Philip Withnall from comment #102)
> have you run all the new tests through valgrind?

Not so far (I can't even remember whether we have infrastructure to do that).
Comment 107 Simon McVittie 2018-07-03 18:52:47 UTC
Created attachment 140452 [details] [review]
[21+] fixup! containers test: NameOwnerChanged is visible  within a container

Use gsize for memory iteration
Comment 108 Simon McVittie 2018-07-03 18:53:35 UTC
Created attachment 140453 [details] [review]
[32+] fixup! containers test: Check that unsolicited replies  can't be sent

* Use const for array of serial numbers
* Fix several leaks
Comment 109 Simon McVittie 2018-07-03 18:54:50 UTC
Created attachment 140454 [details] [review]
[38+] fixup! containers test: Add a test for sending and receiving method calls

Fix off-by-one indentation
Comment 110 Simon McVittie 2018-07-03 19:00:57 UTC
(In reply to Simon McVittie from comment #105)
> You're right that I should run this
> branch through a coverage check though.

Notable missing coverage in bus/containers.c:

- bus_containers_check_can_activate() doesn't reject a message

- bus_containers_check_can_receive() doesn't seem to reject a broadcast
  other than NameOwnerChanged, which needs investigation, because I thought
  I'd tested that; maybe the match rule isn't there?

- bus_containers_check_can_receive() doesn't seem to accept a broadcast
  from inside the same container, likewise

- bus_containers_check_can_see_well_known_name() doesn't hit the special
  case for DBUS_SERVICE_DBUS being visible

and not specifically related to this feature:

- bus_container_instance_lost_connection() doesn't seem to be hit
  (it should be getting called when the confined connection closes)
Comment 111 Simon McVittie 2018-07-03 19:05:49 UTC
(In reply to Simon McVittie from comment #110)
> and not specifically related to this feature:
> 
> - bus_container_instance_lost_connection() doesn't seem to be hit
>   (it should be getting called when the confined connection closes)

That one at least is easy to explain: when the confined connection closes (in non-OOM code paths), we execute code that is almost equivalent to bus_container_instance_lost_connection(), although not quite (it explicitly frees the data slot on the connection that marks it as being a member of a container).
Comment 112 Philip Withnall 2018-07-08 10:50:12 UTC
Comment on attachment 140452 [details] [review]
[21+] fixup! containers test: NameOwnerChanged is visible  within a container

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

r+
Comment 113 Philip Withnall 2018-07-08 10:50:37 UTC
Comment on attachment 140453 [details] [review]
[32+] fixup! containers test: Check that unsolicited replies  can't be sent

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

r+
Comment 114 Philip Withnall 2018-07-08 10:51:08 UTC
Comment on attachment 140454 [details] [review]
[38+] fixup! containers test: Add a test for sending and receiving method calls

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

r+, most trivially
Comment 115 Philip Withnall 2018-07-08 10:53:09 UTC
(In reply to Simon McVittie from comment #110)
> (In reply to Simon McVittie from comment #105)
> > You're right that I should run this
> > branch through a coverage check though.
> 
> Notable missing coverage in bus/containers.c:
> 
> - bus_containers_check_can_activate() doesn't reject a message
> 
> - bus_containers_check_can_receive() doesn't seem to reject a broadcast
>   other than NameOwnerChanged, which needs investigation, because I thought
>   I'd tested that; maybe the match rule isn't there?
> 
> - bus_containers_check_can_receive() doesn't seem to accept a broadcast
>   from inside the same container, likewise
> 
> - bus_containers_check_can_see_well_known_name() doesn't hit the special
>   case for DBUS_SERVICE_DBUS being visible

OK. I’d ideally like to see tests for these before this all gets merged (in case the tests find bugs), but that’s your call. I’d be almost as happy for these new tests to be a follow-up bug instead.

(In reply to Simon McVittie from comment #111)
> (In reply to Simon McVittie from comment #110)
> > and not specifically related to this feature:
> > 
> > - bus_container_instance_lost_connection() doesn't seem to be hit
> >   (it should be getting called when the confined connection closes)
> 
> That one at least is easy to explain: when the confined connection closes
> (in non-OOM code paths), we execute code that is almost equivalent to
> bus_container_instance_lost_connection(), although not quite (it explicitly
> frees the data slot on the connection that marks it as being a member of a
> container).

Would it make sense to factor out the common code? I haven’t checked.
Comment 116 Simon McVittie 2018-07-11 17:30:43 UTC
I can't see anything r- that I haven't already fixed. Is the remaining negative review for the fact that the coverage doesn't match what it ought to be, or have I missed a problem report?

(In reply to Philip Withnall from comment #115)
> I’d ideally like to see tests for these before this all gets merged (in
> case the tests find bugs), but that’s your call.

Step 1 is to get coverage stats for the parts of Containers that already exist on master, I think.

> (In reply to Simon McVittie from comment #111)
> > That one at least is easy to explain: when the confined connection closes
> > (in non-OOM code paths), we execute code that is almost equivalent to
> > bus_container_instance_lost_connection(), although not quite (it explicitly
> > frees the data slot on the connection that marks it as being a member of a
> > container).
> 
> Would it make sense to factor out the common code? I haven’t checked.

Probably, but I'd have to think carefully about what the common code ought to do, and what the worst-case scenario would be if a reference to a connection was leaked (as defensive programming, if we somehow process a message from a connection that should have been cleaned up already, we'll want to make sure it's still considered to be part of its container and doesn't get to escalate privileges to look like an uncontained connection).

That code is not new in this bug anyway.
Comment 117 Philip Withnall 2018-07-11 17:42:14 UTC
(In reply to Simon McVittie from comment #116)
> I can't see anything r- that I haven't already fixed. Is the remaining
> negative review for the fact that the coverage doesn't match what it ought
> to be, or have I missed a problem report?

Er, I think it’s a mistake on my part. It’s review+ as far as I’m concerned; it’s up to you about what to do with the coverage/missing tests.
Comment 118 Simon McVittie 2018-07-11 18:22:17 UTC
(In reply to Simon McVittie from comment #106)
> (In reply to Philip Withnall from comment #102)
> > have you run all the new tests through valgrind?
> 
> Not so far (I can't even remember whether we have infrastructure to do that).

We didn't. Bug #107194 is a start towards this.
Comment 119 Simon McVittie 2018-08-29 18:31:55 UTC
(In reply to Simon McVittie from comment #116)
> > (In reply to Simon McVittie from comment #111)
> > > That one at least is easy to explain: when the confined connection closes
> > > (in non-OOM code paths), we execute code that is almost equivalent to
> > > bus_container_instance_lost_connection(), although not quite (it explicitly
> > > frees the data slot on the connection that marks it as being a member of a
> > > container).
> > 
> > Would it make sense to factor out the common code? I haven’t checked.
> 
> Probably, but I'd have to think carefully about what the common code ought
> to do, and what the worst-case scenario would be if a reference to a
> connection was leaked (as defensive programming, if we somehow process a
> message from a connection that should have been cleaned up already, we'll
> want to make sure it's still considered to be part of its container and
> doesn't get to escalate privileges to look like an uncontained connection).

See Bug #107739 for this.
Comment 120 Simon McVittie 2018-08-29 18:55:09 UTC
Created attachment 141363 [details] [review]
[21+] fixup! containers test: NameOwnerChanged is visible within a container

---

Avoid a g_queue_foreach() idiom that gcc 8 doesn't like, similar to Bug #107349
Comment 121 Simon McVittie 2018-08-29 18:56:06 UTC
Created attachment 141364 [details] [review]
[28+] fixup! containers test: Exercise name-listing methods

---

Fix misuse of G_N_ELEMENTS on a GStrv (thanks, gcc 8; I take it all
back, your new warnings *are* useful)
Comment 122 Simon McVittie 2018-08-29 19:23:29 UTC
(In reply to Simon McVittie from comment #110)
> - bus_containers_check_can_activate() doesn't reject a message

Fix in progress

> - bus_containers_check_can_receive() doesn't seem to reject a broadcast
>   other than NameOwnerChanged, which needs investigation, because I thought
>   I'd tested that; maybe the match rule isn't there?

TODO

> - bus_containers_check_can_receive() doesn't seem to accept a broadcast
>   from inside the same container, likewise

TODO

> - bus_containers_check_can_see_well_known_name() doesn't hit the special
>   case for DBUS_SERVICE_DBUS being visible

That special case is never hit, because all callers already special-cased DBUS_SERVICE_DBUS; I'll remove the special-case here.

There is also missing coverage for a contained connection inspecting an uncontained connection ("inspecting" means GetConnectionCredentials etc.) but I can't test that until I implement SEE access, because at the moment the method call is rejected early for trying to learn whether a disallowed well-known or unique name exists, before we get as far as seeing whether we're allowed to inspect it.
Comment 123 Philip Withnall 2018-08-30 09:40:12 UTC
Comment on attachment 141363 [details] [review]
[21+] fixup! containers test: NameOwnerChanged is visible within a container

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

r+
Comment 124 Philip Withnall 2018-08-30 09:40:50 UTC
Comment on attachment 141364 [details] [review]
[28+] fixup! containers test: Exercise name-listing methods

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

r+
Comment 125 Simon McVittie 2018-08-30 14:45:47 UTC
Created attachment 141378 [details] [review]
[26+] fixup! containers: Containers with an Allow policy  cannot see well-known names

---

bus_containers_check_can_see_well_known_name: Don't handle DBUS_SERVICE_DBUS

Callers of this function have a fast path for DBUS_SERVICE_DBUS anyway.

This kills off some untested code.
Comment 126 Simon McVittie 2018-08-30 14:51:19 UTC
Created attachment 141379 [details] [review]
[38+] fixup! containers test: Add a test for sending and  receiving method calls

Expand coverage for activation
Comment 127 Simon McVittie 2018-08-30 14:51:47 UTC
Created attachment 141380 [details] [review]
[38+] fixup! containers test: Add a test for sending and  receiving method calls

Expand test coverage for inspecting other connections

These all raise NameHasNoOwner without checking whether we are
allowed to inspect the other connection, because we are not even
allowed to see the other connection. After implementing fd.o#105658,
in an equivalent test where we are allowed to see the unconfined
connection but not inspect it, they should raise AccessDenied.
Comment 128 Simon McVittie 2018-08-30 15:06:29 UTC
Created attachment 141382 [details] [review]
[39 v2] containers test: Exercise allowing/denying signals

---

This is a rewrite rather than a fixup, because it turns out the signals test was completely broken: SIGNAL_SENT was numerically equal to SIGNAL_INVALID, the array terminator. So we didn't actually do any iterations, which is why we had no test coverage for broadcasts. /o\

I've taken the opportunity to redo it to be more obviously correct: instead of inferring in code who should receive which signals from whether the signal is a broadcast, I've made it explicit via SIGNAL_RECEIVED_OUTSIDE and/or SIGNAL_RECEIVED_INSIDE in the test-case struct.

My apologies for the re-review, but I think it's clearer this way.

Test logs for the "most privileged" case:

# Unconfined connection: ":1.0"
# Unconfined observer connection: ":1.1"
# Calling AddServer...
# Connecting to unix:path=/tmp/dbus-test-containers.VLTFOZ/dbus-1/containers/dbus-wb3lyMjBR5,guid=fa14aed9122e9c5477e4cf0b5b87ff88...
# Confined connection 0: ":1.3"
# Connecting to unix:path=/tmp/dbus-test-containers.VLTFOZ/dbus-1/containers/dbus-wb3lyMjBR5,guid=fa14aed9122e9c5477e4cf0b5b87ff88...
# Confined connection 1: ":1.4" owns "com.example.Confined"
# test_allow_signals omit-allow #0
# confined connection sending signal
# ... unicast to :confined[1] (:1.4)
# ... path /
# ... com.example.Foo.UnicastSignal
# Synchronizing caller 0x55a1ae474860 with callee 0x55a1ae474a60...
# Unconfined connection saw unconfined observer connection ":1.1" appear, owned by ":1.1"
# Unconfined connection saw unconfined observer connection "com.example.Observer" disappear
# Confined connection saw NameOwnerChanged: ":1.4" owner "" -> ":1.4"
# Confined connection saw NameOwnerChanged: "com.example.Confined" owner "" -> ":1.4"
# Confined connection saw NameOwnerChanged: "com.example.Unconfined" owner "" -> ":1.0"
# Confined connection saw NameOwnerChanged: "com.example.Observer" owner "" -> ":1.1"
# Unconfined connection saw unconfined observer connection "com.example.Observer" appear, owned by ":1.1"
# Connection 0x55a1ae474a60 received com.example.Foo.UnicastSignal from :1.3:/
# Synchronized caller 0x55a1ae474860 with callee 0x55a1ae474a60
# -> 1 signal(s) received by confined connection 1 <0x55a1ae474a60>
# test_allow_signals omit-allow #1
# unconfined connection sending signal
# ... unicast to :confined (:1.3)
# ... path /
# ... com.example.Foo.UnicastSignal
# Synchronizing caller 0x55a1ae4a8210 with callee 0x55a1ae474e60...
# Connection 0x55a1ae474860 received com.example.Foo.UnicastSignal from :1.0:/
# Synchronized caller 0x55a1ae4a8210 with callee 0x55a1ae474e60
# Synchronizing caller 0x55a1ae4a8210 with callee 0x55a1ae474860...
# Synchronized caller 0x55a1ae4a8210 with callee 0x55a1ae474860
# -> 0 signal(s) received by observer <0x55a1ae474e60>
# -> 1 signal(s) received by confined connection 0 <0x55a1ae474860>
# test_allow_signals omit-allow #2
# confined connection sending signal
# ... unicast to :unconfined (:1.0)
# ... path /
# ... com.example.Foo.UnicastSignal
# ... with attached file descriptor
# Synchronizing caller 0x55a1ae4a8210 with callee 0x55a1ae474860...
# Connection 0x55a1ae4a8210 received com.example.Foo.UnicastSignal from :1.3:/
# Synchronized caller 0x55a1ae4a8210 with callee 0x55a1ae474860
# -> 1 signal(s) received by unconfined connection <0x55a1ae4a8210>
# test_allow_signals omit-allow #3
# unconfined connection sending signal
# ... unicast to :confined (:1.3)
# ... path /
# ... com.example.Foo.UnicastSignal
# ... with attached file descriptor
# Synchronizing caller 0x55a1ae4a8210 with callee 0x55a1ae474e60...
# Connection 0x55a1ae474860 received com.example.Foo.UnicastSignal from :1.0:/
# Synchronized caller 0x55a1ae4a8210 with callee 0x55a1ae474e60
# Synchronizing caller 0x55a1ae4a8210 with callee 0x55a1ae474860...
# Synchronized caller 0x55a1ae4a8210 with callee 0x55a1ae474860
# -> 0 signal(s) received by observer <0x55a1ae474e60>
# -> 1 signal(s) received by confined connection 0 <0x55a1ae474860>
# test_allow_signals omit-allow #4
# unconfined connection sending signal
# ... broadcast
# ... path /
# ... com.example.Foo.BroadcastSignal
# Synchronizing caller 0x55a1ae4a8210 with callee 0x55a1ae474e60...
# Connection 0x55a1ae474860 received com.example.Foo.BroadcastSignal from :1.0:/
# Connection 0x55a1ae474e60 received com.example.Foo.BroadcastSignal from :1.0:/
# Synchronized caller 0x55a1ae4a8210 with callee 0x55a1ae474e60
# Synchronizing caller 0x55a1ae4a8210 with callee 0x55a1ae474860...
# Synchronized caller 0x55a1ae4a8210 with callee 0x55a1ae474860
# -> 1 signal(s) received by observer <0x55a1ae474e60>
# -> 1 signal(s) received by confined connection 0 <0x55a1ae474860>
# test_allow_signals omit-allow #5
# confined connection sending signal
# ... unicast to :unconfined (:1.0)
# ... path /
# ... com.example.Foo.UnicastSignal
# Synchronizing caller 0x55a1ae4a8210 with callee 0x55a1ae474860...
# Connection 0x55a1ae4a8210 received com.example.Foo.UnicastSignal from :1.3:/
# Synchronized caller 0x55a1ae4a8210 with callee 0x55a1ae474860
# -> 1 signal(s) received by unconfined connection <0x55a1ae4a8210>
# test_allow_signals omit-allow #6
# confined connection sending signal
# ... broadcast
# ... path /
# ... com.example.Foo.BroadcastSignal
# Synchronizing caller 0x55a1ae474e60 with callee 0x55a1ae474860...
# Connection 0x55a1ae474e60 received com.example.Foo.BroadcastSignal from :1.3:/
# Connection 0x55a1ae474a60 received com.example.Foo.BroadcastSignal from :1.3:/
# Synchronized caller 0x55a1ae474e60 with callee 0x55a1ae474860
# Synchronizing caller 0x55a1ae474860 with callee 0x55a1ae474a60...
# Synchronized caller 0x55a1ae474860 with callee 0x55a1ae474a60
# -> 1 signal(s) received by observer <0x55a1ae474e60>
# -> 1 signal(s) received by confined connection 1 <0x55a1ae474a60>
ok 25 /containers/allow/omit-allow/signals
PASS: test-containers 25 /containers/allow/omit-allow/signals

and the "least privileged" case:

# Unconfined connection: ":1.0"
# Unconfined observer connection: ":1.1"
# Calling AddServer...
# Connecting to unix:path=/tmp/dbus-test-containers.VLTFOZ/dbus-1/containers/dbus-eyRRoneF0B,guid=790cf3a46ed83e5b8caf41905b87ff89...
# Confined connection 0: ":1.3"
# Connecting to unix:path=/tmp/dbus-test-containers.VLTFOZ/dbus-1/containers/dbus-eyRRoneF0B,guid=790cf3a46ed83e5b8caf41905b87ff89...
# Confined connection 1: ":1.4"
# test_allow_signals empty-allow #0
# confined connection sending signal
# ... unicast to :confined[1] (:1.4)
# ... path /
# ... com.example.Foo.UnicastSignal
# Synchronizing caller 0x55a1ae4a8610 with callee 0x55a1ae4a8510...
# Unconfined connection saw unconfined observer connection ":1.1" appear, owned by ":1.1"
# Unconfined connection saw unconfined observer connection "com.example.Observer" disappear
# Confined connection saw NameOwnerChanged: ":1.4" owner "" -> ":1.4"
# Unconfined connection saw unconfined observer connection "com.example.Observer" appear, owned by ":1.1"
# Connection 0x55a1ae4a8510 received com.example.Foo.UnicastSignal from :1.3:/
# Synchronized caller 0x55a1ae4a8610 with callee 0x55a1ae4a8510
# -> 1 signal(s) received by confined connection 1 <0x55a1ae4a8510>
# test_allow_signals empty-allow #1
# unconfined connection sending signal
# ... unicast to :confined (:1.3)
# ... path /
# ... com.example.Foo.UnicastSignal
# Synchronizing caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8b10...
# Connection 0x55a1ae4a8610 received com.example.Foo.UnicastSignal from :1.0:/
# Synchronized caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8b10
# Synchronizing caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8610...
# Synchronized caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8610
# -> 0 signal(s) received by observer <0x55a1ae4a8b10>
# -> 1 signal(s) received by confined connection 0 <0x55a1ae4a8610>
# test_allow_signals empty-allow #2
# confined connection sending signal
# ... unicast to :unconfined (:1.0)
# ... path /
# ... com.example.Foo.UnicastSignal
# ... with attached file descriptor
# Synchronizing caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8610...
# Synchronized caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8610
# -> 0 signal(s) received by unconfined connection <0x55a1ae4a8e10>
# test_allow_signals empty-allow #3
# unconfined connection sending signal
# ... unicast to :confined (:1.3)
# ... path /
# ... com.example.Foo.UnicastSignal
# ... with attached file descriptor
# Synchronizing caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8b10...
# Synchronized caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8b10
# Synchronizing caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8610...
# Synchronized caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8610
# -> 0 signal(s) received by observer <0x55a1ae4a8b10>
# -> 0 signal(s) received by confined connection 0 <0x55a1ae4a8610>
# test_allow_signals empty-allow #4
# unconfined connection sending signal
# ... broadcast
# ... path /
# ... com.example.Foo.BroadcastSignal
# Synchronizing caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8b10...
# Connection 0x55a1ae4a8b10 received com.example.Foo.BroadcastSignal from :1.0:/
# Synchronized caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8b10
# Synchronizing caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8610...
# Synchronized caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8610
# -> 1 signal(s) received by observer <0x55a1ae4a8b10>
# -> 0 signal(s) received by confined connection 0 <0x55a1ae4a8610>
# test_allow_signals empty-allow #5
# confined connection sending signal
# ... unicast to :unconfined (:1.0)
# ... path /
# ... com.example.Foo.UnicastSignal
# Synchronizing caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8610...
# Synchronized caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8610
# -> 0 signal(s) received by unconfined connection <0x55a1ae4a8e10>
# test_allow_signals empty-allow #6
# confined connection sending signal
# ... broadcast
# ... path /
# ... com.example.Foo.BroadcastSignal
# Synchronizing caller 0x55a1ae4a8b10 with callee 0x55a1ae4a8610...
# Connection 0x55a1ae4a8510 received com.example.Foo.BroadcastSignal from :1.3:/
# Synchronized caller 0x55a1ae4a8b10 with callee 0x55a1ae4a8610
# Synchronizing caller 0x55a1ae4a8610 with callee 0x55a1ae4a8510...
# Synchronized caller 0x55a1ae4a8610 with callee 0x55a1ae4a8510
# -> 0 signal(s) received by observer <0x55a1ae4a8b10>
# -> 1 signal(s) received by confined connection 1 <0x55a1ae4a8510>
ok 31 /containers/allow/empty-allow/signals
PASS: test-containers 31 /containers/allow/empty-allow/signals
Comment 129 Philip Withnall 2018-09-04 09:45:55 UTC
Comment on attachment 141378 [details] [review]
[26+] fixup! containers: Containers with an Allow policy  cannot see well-known names

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

> Callers of this function have a fast path for DBUS_SERVICE_DBUS anyway.

Really? bus_activation_list_services() doesn’t seem to. Neither does send_one_message() or bus_driver_get_owner_of_name() or bus_registry_list_services().

I’m erring more on the side of either keeping the check in this function, or explicitly asserting that `strcmp (name, DBUS_SERVICE_DBUS) != 0` and requiring the caller to handle DBUS_SERVICE_DBUS explicitly itself. I’m not so happy about the middle ground where DBUS_SERVICE_DBUS could get forgotten about on the caller and callee sides.
Comment 130 Philip Withnall 2018-09-04 09:46:45 UTC
Comment on attachment 141379 [details] [review]
[38+] fixup! containers test: Add a test for sending and  receiving method calls

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

r+
Comment 131 Philip Withnall 2018-09-04 09:47:09 UTC
Comment on attachment 141380 [details] [review]
[38+] fixup! containers test: Add a test for sending and  receiving method calls

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

r+
Comment 132 Philip Withnall 2018-09-04 09:53:36 UTC
Comment on attachment 141382 [details] [review]
[39 v2] containers test: Exercise allowing/denying signals

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

r+ with one nitpick

::: test/containers.c
@@ +4098,5 @@
> +      else if (g_strcmp0 (signal->bus_name,
> +                          REPLACE_WITH_CONFINED_1_UNIQUE_NAME) == 0)
> +        {
> +          /* It's unicast to the other confined connection */
> +          g_assert ((signal->result & SIGNAL_RECEIVED_OUTSIDE) == 0);

Nitpick: I’d use g_assert_cmpuint() for this, partially because it gives a more descriptive failure message; and partially because g_assert() can be compiled out when compiling with G_DISABLE_ASSERT, which wouldn’t be so good for the unit tests. g_assert_*() cannot.
Comment 133 GitLab Migration User 2018-10-12 21:34:09 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/201.

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.