Bug 33474

Summary:	RLIMIT_NOFILE is typically less than our claimed max number of connections
Product:	dbus	Reporter:	Simon McVittie <smcv>
Component:	core	Assignee:	Colin Walters <walters>
Status:	RESOLVED FIXED	QA Contact:	John (J5) Palmieri <johnp>
Severity:	major
Priority:	high	CC:	hp, smcv, thiago, walters
Version:	unspecified	Keywords:	patch
Hardware:	All
OS:	All
Whiteboard:	review-
i915 platform:		i915 features:
Bug Depends on:	23194
Bug Blocks:
Attachments:	raise fd limit

Description Simon McVittie 2011-01-25 07:49:13 UTC

+++ This bug was initially created as a clone of Bug #23194 +++

On that bug, Colin wrote:

"""
I agree that the poll() code here is busted, however I think there's also a
larger issue, which is the desynchronization between the process ulimit for the
bus, and its configuration.

Many Linux forks and embeds use 1024 as a per-process default I believe. 
However the default bus configuration we ship has:

  parser->limits.max_completed_connections = 2048;

In addition to:

  parser->limits.max_incomplete_connections = 64;

For completeness there is also:

  parser->limits.max_connections_per_user = 256;

So:

* Should the system bus instance e.g. ask for more via setrlimit() on startup?
  We'd have to do this before we called setuid(), since on Linux we need
CAP_SYS_RESOURCE.
* Should we change the hardcoded defaults here to be below 1024?
"""

Comment 1 Colin Walters 2011-01-31 13:32:25 UTC

Created attachment 42779 [details] [review]
raise fd limit

Comment 2 Simon McVittie 2011-02-01 03:34:06 UTC

Review of attachment 42779 [details] [review]:

Looks promising, some minor things.

::: dbus/dbus-sysdeps-util-unix.c
@@ +43,3 @@
 #include <fcntl.h>
 #include <sys/stat.h>
+#include <sys/resource.h>

Is this universal on all Unixes, even rubbish ones? I'd prefer it conditionalized with #ifdef HAVE_SYS_RESOURCE_H (for which you need to add it to an existing AC_CHECK_HEADERS, or add a new one).

@@ +407,3 @@
+  /* Ignore "maximum limit", assume we have the "superuser"
+   * privileges.  On Linux this is CAP_SYS_RESOURCE.
+   */

Might be worth clarifying this comment to say that yes, we're ignoring errors, and that's deliberate.

Comment 3 Colin Walters 2011-02-03 10:38:59 UTC

Fixed both things, thanks!

commit 66a09fa7c3c8e4232b4225c49d01d9efb97458c9
Author: Colin Walters <walters@verbum.org>
Date:   Mon Jan 31 15:22:14 2011 -0500

    bus: Raise file descriptor limit to match configuration
    
    The default configuration has hardcoded 2048 complete connections,
    and 64 incomplete.  We need at least that number of file descriptors,
    plus some for internal use.
    
    In the bus, attempt to call setrlimit() before we drop privileges.
    Practically speaking for this means the system bus gets it, the
    session bus doesn't.
    
    http://bugs.freedesktop.org/show_bug.cgi?id=33474
    
    Reviewed-By: Simon McVittie <simon.mcvittie@collabora.co.uk>

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.