Attachment 124841 Details for Bug 95263 – WiP: only read one message at a time if there are fds pending

[patch] WiP: only read one message at a time if there are fds pending

0001-WiP-only-read-one-message-at-a-time-if-there-are-fds.patch (text/plain), 10.92 KB, created by Simon McVittie on 2016-07-01 15:30:37 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2016-07-01 15:30:37 UTC

Size: 10.92 KB

patch

obsolete

>From 6df392436a092a364c6afa64af7ef19305526563 Mon Sep 17 00:00:00 2001
>From: Simon McVittie <simon.mcvittie@collabora.co.uk>
>Date: Fri, 1 Jul 2016 14:46:02 +0100
>Subject: [PATCH] WiP: only read one message at a time if there are fds pending
>
>---
> dbus/dbus-message-internal.h |  4 ++-
> dbus/dbus-message-util.c     | 10 +++---
> dbus/dbus-message.c          | 86 ++++++++++++++++++++++++++++++++++++++++++--
> dbus/dbus-transport-socket.c | 20 ++++++++---
> dbus/dbus-transport.c        |  8 +++--
> 5 files changed, 113 insertions(+), 15 deletions(-)
>
>diff --git a/dbus/dbus-message-internal.h b/dbus/dbus-message-internal.h
>index 4bb4d8b..386bc50 100644
>--- a/dbus/dbus-message-internal.h
>+++ b/dbus/dbus-message-internal.h
>@@ -73,7 +73,9 @@ void               _dbus_message_loader_unref                 (DBusMessageLoader
> 
> DBUS_PRIVATE_EXPORT
> void               _dbus_message_loader_get_buffer            (DBusMessageLoader  *loader,
>-                                                               DBusString        **buffer);
>+                                                               DBusString        **buffer,
>+                                                               int                *max_to_read,
>+                                                               dbus_bool_t        *may_read_unix_fds);
> DBUS_PRIVATE_EXPORT
> void               _dbus_message_loader_return_buffer         (DBusMessageLoader  *loader,
>                                                                DBusString         *buffer);
>diff --git a/dbus/dbus-message-util.c b/dbus/dbus-message-util.c
>index a9d4a51..e226223 100644
>--- a/dbus/dbus-message-util.c
>+++ b/dbus/dbus-message-util.c
>@@ -489,7 +489,7 @@ dbus_internal_do_not_use_try_message_data (const DBusString    *data,
>     {
>       DBusString *buffer;
> 
>-      _dbus_message_loader_get_buffer (loader, &buffer);
>+      _dbus_message_loader_get_buffer (loader, &buffer, NULL, NULL);
>       _dbus_string_append_byte (buffer,
>                                 _dbus_string_get_byte (data, i));
>       _dbus_message_loader_return_buffer (loader, buffer);
>@@ -508,7 +508,7 @@ dbus_internal_do_not_use_try_message_data (const DBusString    *data,
>   {
>     DBusString *buffer;
> 
>-    _dbus_message_loader_get_buffer (loader, &buffer);
>+    _dbus_message_loader_get_buffer (loader, &buffer, NULL, NULL);
>     _dbus_string_copy (data, 0, buffer,
>                        _dbus_string_get_length (buffer));
>     _dbus_message_loader_return_buffer (loader, buffer);
>@@ -529,7 +529,7 @@ dbus_internal_do_not_use_try_message_data (const DBusString    *data,
>     {
>       DBusString *buffer;
> 
>-      _dbus_message_loader_get_buffer (loader, &buffer);
>+      _dbus_message_loader_get_buffer (loader, &buffer, NULL, NULL);
>       _dbus_string_append_byte (buffer,
>                                 _dbus_string_get_byte (data, i));
>       if ((i+1) < len)
>@@ -1497,7 +1497,7 @@ _dbus_message_test (const char *test_data_dir)
>     {
>       DBusString *buffer;
> 
>-      _dbus_message_loader_get_buffer (loader, &buffer);
>+      _dbus_message_loader_get_buffer (loader, &buffer, NULL, NULL);
>       _dbus_string_append_byte (buffer, data[i]);
>       _dbus_message_loader_return_buffer (loader, buffer);
>     }
>@@ -1508,7 +1508,7 @@ _dbus_message_test (const char *test_data_dir)
>     {
>       DBusString *buffer;
> 
>-      _dbus_message_loader_get_buffer (loader, &buffer);
>+      _dbus_message_loader_get_buffer (loader, &buffer, NULL, NULL);
>       _dbus_string_append_byte (buffer, data[i]);
>       _dbus_message_loader_return_buffer (loader, buffer);
>     }
>diff --git a/dbus/dbus-message.c b/dbus/dbus-message.c
>index e22fe51..44640fe 100644
>--- a/dbus/dbus-message.c
>+++ b/dbus/dbus-message.c
>@@ -4033,13 +4033,95 @@ _dbus_message_loader_unref (DBusMessageLoader *loader)
>  */
> void
> _dbus_message_loader_get_buffer (DBusMessageLoader  *loader,
>-                                 DBusString        **buffer)
>+                                 DBusString        **buffer,
>+                                 int                *max_to_read,
>+                                 dbus_bool_t        *may_read_fds)
> {
>   _dbus_assert (!loader->buffer_outstanding);
> 
>   *buffer = &loader->data;
> 
>   loader->buffer_outstanding = TRUE;
>+
>+  if (max_to_read != NULL)
>+    {
>+      int offset = 0;
>+      int remain;
>+      int byte_order;
>+      int fields_array_len;
>+      int header_len;
>+      int body_len;
>+
>+      *max_to_read = DBUS_MAXIMUM_MESSAGE_LENGTH;
>+      *may_read_fds = TRUE;
>+
>+      /* If we aren't holding onto any fds, we can read as much as we want
>+       * (fast path). */
>+      if (loader->n_unix_fds == 0)
>+        return;
>+
>+      /* Slow path: we have a message with some fds in it. We don't want
>+       * to start on the next message until this one is out of the way;
>+       * otherwise a legitimate sender can keep us processing messages
>+       * containing fds, until we disconnect it for having had fds pending
>+       * for too long, a limit that is in place to stop malicious senders
>+       * from setting up recursive fd-passing that takes up our quota and
>+       * will never go away. */
>+
>+      remain = _dbus_string_get_length (&loader->data);
>+
>+      while (remain > 0)
>+        {
>+          DBusValidity validity = DBUS_VALIDITY_UNKNOWN;
>+          int needed;
>+
>+          /* If 0 < remain < DBUS_MINIMUM_HEADER_SIZE, then we've had at
>+           * least the first byte of a message, but we don't know how
>+           * much more to read. Only read the rest of the
>+           * DBUS_MINIMUM_HEADER_SIZE for now; then we'll know. */
>+          if (remain < DBUS_MINIMUM_HEADER_SIZE)
>+            {
>+              *max_to_read = DBUS_MINIMUM_HEADER_SIZE - remain;
>+              *may_read_fds = FALSE;
>+              return;
>+            }
>+
>+          if (!_dbus_header_have_message_untrusted (loader->max_message_size,
>+                                                    &validity,
>+                                                    &byte_order,
>+                                                    &fields_array_len,
>+                                                    &header_len,
>+                                                    &body_len,
>+                                                    &loader->data,
>+                                                    offset,
>+                                                    remain))
>+            {
>+              /* If a message in the buffer is invalid, we're going to
>+               * disconnect the sender anyway, so reading an arbitrary amount
>+               * is fine. */
>+              if (validity != DBUS_VALID)
>+                return;
>+
>+              /* We have a partial message, with the
>+               * DBUS_MINIMUM_HEADER_SIZE-byte fixed part of the header (which
>+               * lets us work out how much more we need), but no more. Read
>+               * the rest of the message. */
>+              needed = header_len + body_len;
>+              _dbus_assert (needed > remain);
>+              *max_to_read = needed - remain;
>+              *may_read_fds = FALSE;
>+              return;
>+            }
>+
>+          /* Skip over entire messages until we have less than a message
>+           * remaining. */
>+          needed = header_len + body_len;
>+          _dbus_assert (needed > DBUS_MINIMUM_HEADER_SIZE);
>+          _dbus_assert (remain >= needed);
>+          remain -= needed;
>+          offset += needed;
>+        }
>+    }
> }
> 
> /**
>@@ -4868,7 +4950,7 @@ dbus_message_demarshal (const char *str,
>   if (loader == NULL)
>     return NULL;
> 
>-  _dbus_message_loader_get_buffer (loader, &buffer);
>+  _dbus_message_loader_get_buffer (loader, &buffer, NULL, NULL);
> 
>   if (!_dbus_string_append_len (buffer, str, len))
>     goto fail_oom;
>diff --git a/dbus/dbus-transport-socket.c b/dbus/dbus-transport-socket.c
>index dce5c7d..5b8538e 100644
>--- a/dbus/dbus-transport-socket.c
>+++ b/dbus/dbus-transport-socket.c
>@@ -794,7 +794,9 @@ do_reading (DBusTransport *transport)
>       if (bytes_read > 0)
>         {
>           _dbus_message_loader_get_buffer (transport->loader,
>-                                           &buffer);
>+                                           &buffer,
>+                                           NULL,
>+                                           NULL);
> 
>           if (!_dbus_auth_decode_data (transport->auth,
>                                        &socket_transport->encoded_incoming,
>@@ -817,11 +819,19 @@ do_reading (DBusTransport *transport)
>     }
>   else
>     {
>+      int max_to_read = DBUS_MAXIMUM_MESSAGE_LENGTH;
>+      dbus_bool_t may_read_unix_fds = TRUE;
>+
>       _dbus_message_loader_get_buffer (transport->loader,
>-                                       &buffer);
>+                                       &buffer,
>+                                       &max_to_read,
>+                                       &may_read_unix_fds);
>+
>+      if (max_to_read > socket_transport->max_bytes_read_per_iteration)
>+        max_to_read = socket_transport->max_bytes_read_per_iteration;
> 
> #ifdef HAVE_UNIX_FD_PASSING
>-      if (DBUS_TRANSPORT_CAN_SEND_UNIX_FD(transport))
>+      if (DBUS_TRANSPORT_CAN_SEND_UNIX_FD(transport) && may_read_unix_fds)
>         {
>           int *fds;
>           unsigned int n_fds;
>@@ -836,7 +846,7 @@ do_reading (DBusTransport *transport)
> 
>           bytes_read = _dbus_read_socket_with_unix_fds(socket_transport->fd,
>                                                        buffer,
>-                                                       socket_transport->max_bytes_read_per_iteration,
>+                                                       max_to_read,
>                                                        fds, &n_fds);
>           saved_errno = _dbus_save_socket_errno ();
> 
>@@ -849,7 +859,7 @@ do_reading (DBusTransport *transport)
> #endif
>         {
>           bytes_read = _dbus_read_socket (socket_transport->fd,
>-                                          buffer, socket_transport->max_bytes_read_per_iteration);
>+                                          buffer, max_to_read);
>           saved_errno = _dbus_save_socket_errno ();
>         }
> 
>diff --git a/dbus/dbus-transport.c b/dbus/dbus-transport.c
>index 31586b1..ca31aa3 100644
>--- a/dbus/dbus-transport.c
>+++ b/dbus/dbus-transport.c
>@@ -1029,7 +1029,9 @@ recover_unused_bytes (DBusTransport *transport)
>         }
>       
>       _dbus_message_loader_get_buffer (transport->loader,
>-                                       &buffer);
>+                                       &buffer,
>+                                       NULL,
>+                                       NULL);
>       
>       orig_len = _dbus_string_get_length (buffer);
>       
>@@ -1059,7 +1061,9 @@ recover_unused_bytes (DBusTransport *transport)
>       dbus_bool_t succeeded;
> 
>       _dbus_message_loader_get_buffer (transport->loader,
>-                                       &buffer);
>+                                       &buffer,
>+                                       NULL,
>+                                       NULL);
>                 
>       orig_len = _dbus_string_get_length (buffer);
>                 
>-- 
>2.8.1
>

Actions: View | Splinter Review

Attachments on bug 95263: 123493 | 123506 | 123507 | 123568 | 124840 | 124841 | 124842 | 129002

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.