Attachment 108853 Details for Bug 85105 – [PATCH 2/2] Set fd rlimit to 64k for the system dbus-daemon

[patch] [PATCH 2/2] Set fd rlimit to 64k for the system dbus-daemon

0002-Set-fd-rlimit-to-64k-for-the-system-dbus-daemon.patch (text/plain), 12.30 KB, created by Alban Crequy on 2014-11-03 17:15:10 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Alban Crequy

Created: 2014-11-03 17:15:10 UTC

Size: 12.30 KB

patch

obsolete

>From 02dd0059f2275b79064f0a096ee756882b2819ee Mon Sep 17 00:00:00 2001
>From: Simon McVittie <simon.mcvittie@collabora.co.uk>
>Date: Thu, 16 Oct 2014 15:09:47 +0100
>Subject: [PATCH 2/2] Set fd rlimit to 64k for the system dbus-daemon
>
>Restore the original rlimit for activated services.
>
>[make restore fd limit non-fatal -alban]
>[use LOG_WARNING -alban]
>[use #ifdef DBUS_UNIX -alban]
>
>https://bugs.freedesktop.org/show_bug.cgi?id=85105
>---
> bus/activation.c              |  28 +++++++-
> bus/bus.c                     |  52 ++++++++++++---
> bus/bus.h                     |   1 +
> dbus/dbus-sysdeps-util-unix.c | 145 +++++++++++++++++++++++++++++++++---------
> dbus/dbus-sysdeps-util-win.c  |  29 ++++++++-
> dbus/dbus-sysdeps.h           |   9 +++
> 6 files changed, 222 insertions(+), 42 deletions(-)
>
>diff --git a/bus/activation.c b/bus/activation.c
>index bdf9cf3..9610c04 100644
>--- a/bus/activation.c
>+++ b/bus/activation.c
>@@ -1688,6 +1688,31 @@ out:
>   return retval;
> }
> 
>+static void
>+child_setup (void *user_data)
>+{
>+#ifdef DBUS_UNIX
>+  BusActivation *activation = user_data;
>+  DBusRLimit *initial_fd_limit;
>+  DBusError error;
>+
>+  dbus_error_init (&error);
>+  initial_fd_limit = bus_context_get_initial_fd_limit (activation->context);
>+
>+  if (initial_fd_limit != NULL &&
>+      !_dbus_rlimit_restore_fd_limit (initial_fd_limit, &error))
>+    {
>+      /* unfortunately we don't actually know the service name here */
>+      bus_context_log (activation->context,
>+                       DBUS_SYSTEM_LOG_WARNING,
>+                       "Failed to reset fd limit before activating "
>+                       "service: %s: %s",
>+                       error.name, error.message);
>+    }
>+#endif
>+}
>+
>+
> dbus_bool_t
> bus_activation_activate_service (BusActivation  *activation,
>                                  DBusConnection *connection,
>@@ -2121,7 +2146,8 @@ bus_activation_activate_service (BusActivation  *activation,
>                                           service_name,
>                                           argv,
>                                           envp,
>-                                          NULL, activation,
>+                                          child_setup,
>+                                          activation,
>                                           &tmp_error))
>     {
>       _dbus_verbose ("Failed to spawn child\n");
>diff --git a/bus/bus.c b/bus/bus.c
>index 35d4075..6186697 100644
>--- a/bus/bus.c
>+++ b/bus/bus.c
>@@ -64,6 +64,7 @@ struct BusContext
>   BusPolicy *policy;
>   BusMatchmaker *matchmaker;
>   BusLimits limits;
>+  DBusRLimit *initial_fd_limit;
>   unsigned int fork : 1;
>   unsigned int syslog : 1;
>   unsigned int keep_umask : 1;
>@@ -659,19 +660,38 @@ oom:
> static void
> raise_file_descriptor_limit (BusContext      *context)
> {
>+#ifdef DBUS_UNIX
>+  DBusError error = DBUS_ERROR_INIT;
> 
>-  /* I just picked this out of thin air; we need some extra
>-   * descriptors for things like any internal pipes we create,
>-   * inotify, connections to SELinux, etc.
>-   */
>-  unsigned int arbitrary_extra_fds = 32;
>-  unsigned int limit;
>+  /* we only do this once */
>+  if (context->initial_fd_limit != NULL)
>+    return;
> 
>-  limit = context->limits.max_completed_connections +
>-    context->limits.max_incomplete_connections
>-    + arbitrary_extra_fds;
>+  context->initial_fd_limit = _dbus_rlimit_save_fd_limit (&error);
> 
>-  _dbus_request_file_descriptor_limit (limit);
>+  if (context->initial_fd_limit == NULL)
>+    {
>+      bus_context_log (context, DBUS_SYSTEM_LOG_WARNING,
>+                       "%s: %s", error.name, error.message);
>+      dbus_error_free (&error);
>+      return;
>+    }
>+
>+  /* We used to compute a suitable rlimit based on the configured number
>+   * of connections, but that breaks down as soon as we allow fd-passing,
>+   * because each connection is allowed to pass 64 fds to us, and if
>+   * they all did, we'd hit kernel limits. We now hard-code 64k as a
>+   * good limit, like systemd does: that's enough to avoid DoS from
>+   * anything short of multiple uids conspiring against us.
>+   */
>+  if (!_dbus_rlimit_raise_fd_limit_if_privileged (65536, &error))
>+    {
>+      bus_context_log (context, DBUS_SYSTEM_LOG_WARNING,
>+                       "%s: %s", error.name, error.message);
>+      dbus_error_free (&error);
>+      return;
>+    }
>+#endif
> }
> 
> static dbus_bool_t
>@@ -682,7 +702,9 @@ process_config_postinit (BusContext      *context,
>   DBusHashTable *service_context_table;
>   DBusList *watched_dirs = NULL;
> 
>+#ifdef DBUS_UNIX
>   raise_file_descriptor_limit (context);
>+#endif
> 
>   service_context_table = bus_config_parser_steal_service_context_table (parser);
>   if (!bus_registry_set_service_context_table (context->registry,
>@@ -1130,6 +1152,10 @@ bus_context_unref (BusContext *context)
> 
>           dbus_free (context->pidfile);
> 	}
>+
>+      if (context->initial_fd_limit)
>+        _dbus_rlimit_free (context->initial_fd_limit);
>+
>       dbus_free (context);
> 
>       dbus_server_free_data_slot (&server_data_slot);
>@@ -1294,6 +1320,12 @@ bus_context_get_reply_timeout (BusContext *context)
>   return context->limits.reply_timeout;
> }
> 
>+DBusRLimit *
>+bus_context_get_initial_fd_limit (BusContext *context)
>+{
>+  return context->initial_fd_limit;
>+}
>+
> void
> bus_context_log (BusContext *context, DBusSystemLogSeverity severity, const char *msg, ...) _DBUS_GNUC_PRINTF (3, 4);
> 
>diff --git a/bus/bus.h b/bus/bus.h
>index 7d0b369..dac6ea5 100644
>--- a/bus/bus.h
>+++ b/bus/bus.h
>@@ -116,6 +116,7 @@ int               bus_context_get_max_services_per_connection    (BusContext
> int               bus_context_get_max_match_rules_per_connection (BusContext       *context);
> int               bus_context_get_max_replies_per_connection     (BusContext       *context);
> int               bus_context_get_reply_timeout                  (BusContext       *context);
>+DBusRLimit *      bus_context_get_initial_fd_limit               (BusContext       *context);
> void              bus_context_log                                (BusContext       *context,
>                                                                   DBusSystemLogSeverity severity,
>                                                                   const char       *msg,
>diff --git a/dbus/dbus-sysdeps-util-unix.c b/dbus/dbus-sysdeps-util-unix.c
>index f410806..9b724cc 100644
>--- a/dbus/dbus-sysdeps-util-unix.c
>+++ b/dbus/dbus-sysdeps-util-unix.c
>@@ -380,53 +380,140 @@ _dbus_change_to_daemon_user  (const char    *user,
> }
> #endif /* !HAVE_LIBAUDIT */
> 
>+#ifdef HAVE_SETRLIMIT
> 
>-/**
>- * Attempt to ensure that the current process can open
>- * at least @p limit file descriptors.
>- *
>- * If @p limit is lower than the current, it will not be
>- * lowered.  No error is returned if the request can
>- * not be satisfied.
>- *
>- * @param limit number of file descriptors
>+/* We assume that if we have setrlimit, we also have getrlimit and
>+ * struct rlimit.
>  */
>-void
>-_dbus_request_file_descriptor_limit (unsigned int limit)
>+
>+struct DBusRLimit {
>+    struct rlimit lim;
>+};
>+
>+DBusRLimit *
>+_dbus_rlimit_save_fd_limit (DBusError *error)
>+{
>+  DBusRLimit *self;
>+
>+  self = dbus_new0 (DBusRLimit, 1);
>+
>+  if (self == NULL)
>+    {
>+      _DBUS_SET_OOM (error);
>+      return NULL;
>+    }
>+
>+  if (getrlimit (RLIMIT_NOFILE, &self->lim) < 0)
>+    {
>+      dbus_set_error (error, _dbus_error_from_errno (errno),
>+                      "Failed to get fd limit: %s", _dbus_strerror (errno));
>+      dbus_free (self);
>+      return NULL;
>+    }
>+
>+  return self;
>+}
>+
>+dbus_bool_t
>+_dbus_rlimit_raise_fd_limit_if_privileged (unsigned int  desired,
>+                                           DBusError    *error)
> {
>-#ifdef HAVE_SETRLIMIT
>   struct rlimit lim;
>-  struct rlimit target_lim;
> 
>   /* No point to doing this practically speaking
>    * if we're not uid 0.  We expect the system
>    * bus to use this before we change UID, and
>-   * the session bus takes the Linux default
>-   * of 1024 for both cur and max.
>+   * the session bus takes the Linux default,
>+   * currently 1024 for cur and 4096 for max.
>    */
>   if (getuid () != 0)
>-    return;
>+    {
>+      /* not an error, we're probably the session bus */
>+      return TRUE;
>+    }
> 
>   if (getrlimit (RLIMIT_NOFILE, &lim) < 0)
>-    return;
>+    {
>+      dbus_set_error (error, _dbus_error_from_errno (errno),
>+                      "Failed to get fd limit: %s", _dbus_strerror (errno));
>+      return FALSE;
>+    }
> 
>-  if (lim.rlim_cur >= limit)
>-    return;
>+  if (lim.rlim_cur == RLIM_INFINITY || lim.rlim_cur >= desired)
>+    {
>+      /* not an error, everything is fine */
>+      return TRUE;
>+    }
> 
>   /* Ignore "maximum limit", assume we have the "superuser"
>    * privileges.  On Linux this is CAP_SYS_RESOURCE.
>    */
>-  target_lim.rlim_cur = target_lim.rlim_max = limit;
>-  /* Also ignore errors; if we fail, we will at least work
>-   * up to whatever limit we had, which seems better than
>-   * just outright aborting.
>-   *
>-   * However, in the future we should probably log this so OS builders
>-   * have a chance to notice any misconfiguration like dbus-daemon
>-   * being started without CAP_SYS_RESOURCE.
>-   */
>-  setrlimit (RLIMIT_NOFILE, &target_lim);
>+  lim.rlim_cur = lim.rlim_max = desired;
>+
>+  if (setrlimit (RLIMIT_NOFILE, &lim) < 0)
>+    {
>+      dbus_set_error (error, _dbus_error_from_errno (errno),
>+                      "Failed to set fd limit to %u: %s",
>+                      desired, _dbus_strerror (errno));
>+      return FALSE;
>+    }
>+
>+  return TRUE;
>+}
>+
>+dbus_bool_t
>+_dbus_rlimit_restore_fd_limit (DBusRLimit *saved,
>+                               DBusError  *error)
>+{
>+  if (setrlimit (RLIMIT_NOFILE, &saved->lim) < 0)
>+    {
>+      dbus_set_error (error, _dbus_error_from_errno (errno),
>+                      "Failed to restore old fd limit: %s",
>+                      _dbus_strerror (errno));
>+      return FALSE;
>+    }
>+
>+  return TRUE;
>+}
>+
>+#else /* !HAVE_SETRLIMIT */
>+
>+static void
>+fd_limit_not_supported (DBusError *error)
>+{
>+  dbus_set_error (error, DBUS_ERROR_NOT_SUPPORTED,
>+                  "cannot change fd limit on this platform");
>+}
>+
>+DBusRLimit *
>+_dbus_rlimit_save_fd_limit (DBusError *error)
>+{
>+  fd_limit_not_supported (error);
>+  return NULL;
>+}
>+
>+dbus_bool_t
>+_dbus_rlimit_raise_fd_limit_if_privileged (unsigned int  desired,
>+                                           DBusError    *error)
>+{
>+  fd_limit_not_supported (error);
>+  return FALSE;
>+}
>+
>+dbus_bool_t
>+_dbus_rlimit_restore_fd_limit (DBusRLimit *saved,
>+                               DBusError  *error)
>+{
>+  fd_limit_not_supported (error);
>+  return FALSE;
>+}
>+
> #endif
>+
>+void
>+_dbus_rlimit_free (DBusRLimit *lim)
>+{
>+  dbus_free (lim);
> }
> 
> void
>diff --git a/dbus/dbus-sysdeps-util-win.c b/dbus/dbus-sysdeps-util-win.c
>index dd4b233..1850aae 100644
>--- a/dbus/dbus-sysdeps-util-win.c
>+++ b/dbus/dbus-sysdeps-util-win.c
>@@ -258,9 +258,34 @@ _dbus_change_to_daemon_user  (const char    *user,
>   return TRUE;
> }
> 
>-void
>-_dbus_request_file_descriptor_limit (unsigned int limit)
>+static void
>+fd_limit_not_supported (DBusError *error)
>+{
>+  dbus_set_error (error, DBUS_ERROR_NOT_SUPPORTED,
>+                  "cannot change fd limit on this platform");
>+}
>+
>+DBusRLimit *
>+_dbus_rlimit_save_fd_limit (DBusError *error)
> {
>+  fd_limit_not_supported (error);
>+  return NULL;
>+}
>+
>+dbus_bool_t
>+_dbus_rlimit_raise_fd_limit_if_privileged (unsigned int  desired,
>+                                           DBusError    *error)
>+{
>+  fd_limit_not_supported (error);
>+  return FALSE;
>+}
>+
>+dbus_bool_t
>+_dbus_rlimit_restore_fd_limit (DBusRLimit *saved,
>+                               DBusError  *error)
>+{
>+  fd_limit_not_supported (error);
>+  return FALSE;
> }
> 
> void
>diff --git a/dbus/dbus-sysdeps.h b/dbus/dbus-sysdeps.h
>index a78390e..f122f1b 100644
>--- a/dbus/dbus-sysdeps.h
>+++ b/dbus/dbus-sysdeps.h
>@@ -566,6 +566,15 @@ _dbus_replace_install_prefix (const char *configure_time_path);
>  */
> #define DBUS_DEFAULT_MESSAGE_UNIX_FDS 16
> 
>+typedef struct DBusRLimit DBusRLimit;
>+
>+DBusRLimit     *_dbus_rlimit_save_fd_limit                 (DBusError    *error);
>+dbus_bool_t     _dbus_rlimit_raise_fd_limit_if_privileged  (unsigned int  desired,
>+                                                            DBusError    *error);
>+dbus_bool_t     _dbus_rlimit_restore_fd_limit              (DBusRLimit   *saved,
>+                                                            DBusError    *error);
>+void            _dbus_rlimit_free                          (DBusRLimit   *lim);
>+
> /** @} */
> 
> DBUS_END_DECLS
>-- 
>1.8.5.3
>

Actions: View | Splinter Review

Attachments on bug 85105: 107944 | 108299 | 108848 | 108849 | 108853 | 108886 | 108896 | 108897 | 108898 | 109054

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.