Bug 82346 - add new limit: max_connections_per_process
Summary: add new limit: max_connections_per_process
Status: REOPENED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.5
Hardware: All All
: low enhancement
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review-
Keywords: patch
Depends on:
Blocks:
 
Reported: 2014-08-08 12:34 UTC by Alban Crequy
Modified: 2017-03-24 15:55 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
[PATCH 1/3] config: add new limit: max_connections_per_process (5.82 KB, patch)
2014-08-08 13:01 UTC, Alban Crequy
Details | Splinter Review
[PATCH 2/3] enforce new limit max_connections_per_process (7.87 KB, patch)
2014-08-08 13:02 UTC, Alban Crequy
Details | Splinter Review
[PATCH 3/3] new test: max_connections_per_process (6.13 KB, patch)
2014-08-08 13:03 UTC, Alban Crequy
Details | Splinter Review
[PATCH 3/3] new test: max_connections_per_process (6.40 KB, patch)
2014-08-11 14:36 UTC, Alban Crequy
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alban Crequy 2014-08-08 12:34:36 UTC
dbus-daemon already has the following limits:
- max_completed_connections: (default|system bus)=2048 (session bus)=100000
- max_connections_per_user: (default|system bus)=256 (session bus)=100000

This bug is to add a new limit:
    - max_connections_per_process: (default|system bus)=8 (session bus)=8

Most applications should have only one connection to the bus. But sometimes, a process could legitimately create a few connections to the bus: it happens in an applications with plugins written with different dbus bindings such as libdbus and Qt D-Bus linked in the same process. But there are no good reasons for a process to have plenty of connections.

See also Bug #81469 (max_connections_per_cgroup) for another limit on cgroups.
Comment 1 Alban Crequy 2014-08-08 13:01:52 UTC
Created attachment 104276 [details] [review]
[PATCH 1/3] config: add new limit: max_connections_per_process
Comment 2 Alban Crequy 2014-08-08 13:02:34 UTC
Created attachment 104277 [details] [review]
[PATCH 2/3] enforce new limit max_connections_per_process
Comment 3 Alban Crequy 2014-08-08 13:03:02 UTC
Created attachment 104278 [details] [review]
[PATCH 3/3] new test: max_connections_per_process
Comment 4 Alban Crequy 2014-08-11 14:36:18 UTC
Created attachment 104441 [details] [review]
[PATCH 3/3] new test: max_connections_per_process

v2:
 - fix bug number in test/dbus-daemon.c
 - rename test to max_connections_per_process
Comment 5 Simon McVittie 2014-09-03 18:03:10 UTC
It isn't clear to me why this is useful. Do you intend this to be a security mechanism, or a guard against mistakes, or what?

It seems to me as though this would only have a practical effect if: (pseudocode)

    max_connections_per_process * our RLIMIT_NPROC < max_connections_per_uid
Comment 6 Alban Crequy 2014-09-04 17:52:16 UTC
(In reply to comment #5)
> It isn't clear to me why this is useful. Do you intend this to be a security
> mechanism, or a guard against mistakes, or what?

A guard against mistakes, and also make it slightly more complicated to write exploits. But I agree it is not helping as much as other limits.

> It seems to me as though this would only have a practical effect if:
> (pseudocode)
> 
>     max_connections_per_process * our RLIMIT_NPROC < max_connections_per_uid

In environments using a LSM such as AppArmor, some processes are prevented from connecting to D-Bus so the formula becomes more complex in that case.
Comment 7 Simon McVittie 2014-09-15 15:58:23 UTC
Comment on attachment 104276 [details] [review]
[PATCH 1/3] config: add new limit: max_connections_per_process

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

> But there are no good reasons for a process to have plenty of connections.

o rly? I've seen "one connection per thread" suggested as a way to avoid threading issues when using libraries whose multi-threading does not have a good top-down design (like, er, libdbus).

::: bus/session.conf.in
@@ +55,4 @@
>    <limit name="max_completed_connections">100000</limit>  
>    <limit name="max_incomplete_connections">10000</limit>
>    <limit name="max_connections_per_user">100000</limit>
> +  <limit name="max_connections_per_process">8</limit>

The default limits for the session bus are intended to be "essentially unlimited" (with one exception: fd-passing, for reasons described in the comments).

Also, if you really want the limit for the session bus to be the same as the hard-coded limit in config-parser.c, there's little point in adding this <limit/> to the config file at all.

::: doc/dbus-daemon.1.xml.in
@@ +533,5 @@
>                                       connections
>        "max_connections_per_user"   : max number of completed connections from
>                                       the same user
> +      "max_connections_per_process": max number of completed connections from
> +                                     the same process

"... on platforms where this can be determined reliably"

At the moment that's:

Linux, OpenBSD, other platforms with SO_PEERCRED
FreeBSD, DragonflyBSD, other platforms with SCM_CREDS
Solaris
Windows (but that might be reverted, see bug #83499)

but not, for instance,

Cygwin
Minix
NetBSD (but it might be added, see bug #69702)
OS X
Comment 8 Simon McVittie 2014-09-15 16:09:17 UTC
Comment on attachment 104277 [details] [review]
[PATCH 2/3] enforce new limit max_connections_per_process

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

::: bus/connection.c
@@ +151,5 @@
> +{
> +  void *val;
> +  int current_count;
> +
> +  /* val is NULL is 0 when it isn't in the hash yet */

ISO C does not actually guarantee that.

I don't think we actually care about any platforms where it would be false, but I'd prefer something like

/* val is NULL when it isn't in the hash yet;
 * we assume here that (int) NULL is 0, as it
 * is on any sensible platform
 */

@@ +219,5 @@
> +  _dbus_assert (current_count >= 0);
> +
> +  current_count += adjustment;
> +
> +  _dbus_assert (current_count >= 0);

Careful: signed wraparound is undefined behaviour. In practice it probably wraps around like unsigned numbers do, but ISO C says compilers may assume that it doesn't happen and optimize accordingly.

The practical result is that this is dangerous to rely on.

@@ +523,5 @@
>    connections->context = context;
>    
>    return connections;
>  
> + failed_6:

A label called "failed_6" is a sign that something has gone badly wrong.

Before hacking up this function any further, I would like someone (maybe you, if you have things you want to add here) to refactor it into the form

failed:
  if (connections->pending_replies != NULL)
    bus_expire_list_free (connections->pending_replies);

  if (connections->expire_timeout != NULL)
    _dbus_timeout_unref (connections->expire_timeout);

  /* etc. */

with the appropriate initializations to make that work.

@@ +1616,5 @@
> +
> +  if (d->pid > 0)
> +    {
> +      if (get_connections_for_pid (connections, d->pid) >=
> +          bus_context_get_max_connections_per_process (connections->context))

So... I can bypass this limit by connecting in some way that doesn't reveal my pid, like TCP?

(Not that it could be any other way - if you put all unknown-pid connections into the same "bucket", 8 would be far too few.)
Comment 9 Simon McVittie 2014-09-15 16:11:59 UTC
Comment on attachment 104441 [details] [review]
[PATCH 3/3] new test: max_connections_per_process

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

This test is going to fail on platforms where the pid cannot be determined. OS X and NetBSD seem like the most significant examples.

(test/dbus-daemon.c currently also fails on such platforms, if they're Unix.)
Comment 10 Simon McVittie 2014-09-15 16:16:17 UTC
I'm not really convinced by the usefulness of this feature. In the absence of LSMs etc., processes with the same uid can ptrace each other, or subvert each other in various other ways, whatever you do.
Comment 11 Simon McVittie 2014-09-15 16:20:54 UTC
Comment on attachment 104277 [details] [review]
[PATCH 2/3] enforce new limit max_connections_per_process

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

::: bus/connection.c
@@ +219,5 @@
> +  _dbus_assert (current_count >= 0);
> +
> +  current_count += adjustment;
> +
> +  _dbus_assert (current_count >= 0);

A safer way to do this would be something like (pseudocode):

_dbus_assert (current_count >= 0);

if (adjustment < 0)
  {
    if ((-adjustment) > current_count)
      fail;
  }
else
  {
    if (adjustment > (MAXINT - current_count))
      fail;
  }

where "fail" would probably involve returning FALSE.
Comment 12 Simon McVittie 2014-09-15 16:22:40 UTC
(In reply to comment #9)
> This test is going to fail on platforms where the pid cannot be determined.
> OS X and NetBSD seem like the most significant examples.

I think it would be OK for this test to be run on those platforms where we know that not knowning the pid would be a regression, currently:

#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || \
    defined(__linux__) || \
    defined(__OpenBSD__)
Comment 13 Simon McVittie 2017-03-24 15:55:55 UTC
I think this would be obsoleted by Bug #100344, at which point we could close this WONTFIX.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct.