Attachment 105916 Details for Bug 83625 – Consistently save and restore errno

[patch] Consistently save and restore errno

0001-Consistently-save-and-restore-errno.patch (text/plain), 16.05 KB, created by Simon McVittie on 2014-09-08 19:33:58 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2014-09-08 19:33:58 UTC

Size: 16.05 KB

patch

obsolete

>From a35281a0e8570b94ee36023d45dac0fd53a15c77 Mon Sep 17 00:00:00 2001
>From: Simon McVittie <simon.mcvittie@collabora.co.uk>
>Date: Mon, 8 Sep 2014 20:18:22 +0100
>Subject: [PATCH] Consistently save and restore errno
>
>Some functions in dbus-transport-socket.c make a (wrapped)
>socket syscall, then call other APIs, then test the result and
>errno of the socket syscall.
>
>This would break horribly if those "other APIs" overwrote errno with
>their own value (... and this is part of why errno is an awful API).
>
>Notably, if running under DBUS_VERBOSE, _dbus_verbose() is basically
>fprintf(), which sets errno; and our Unix fd-passing support
>makes calls of the form _dbus_verbose ("Read/wrote %i unix fds\n", n)
>between the syscall and the result processing.
>
>Maybe one day we'll convert all of dbus' syscall wrappers to either
>raise a DBusError, or use the "negative errno" convention that systemd
>borrowed from the Linux kernel, and in particular, we would need to
>do that if we ever ported it to a platform where socket error reporting
>was not basically errno. However, in practice everyone uses something
>derived from BSD sockets, so "this sets errno, you know what errno is"
>is a good enough internal API if we make sure to use it correctly.
>
>Nothing calls _dbus_get_is_errno_nonzero(), so I just removed it instead
>of converting it to the new calling convention.
>---
> dbus/dbus-nonce.c            |  8 ++++++--
> dbus/dbus-server-socket.c    |  7 +++++--
> dbus/dbus-sysdeps-unix.c     | 18 ++++++++++++++---
> dbus/dbus-sysdeps-win.c      | 18 ++++++++++++++---
> dbus/dbus-sysdeps.c          | 30 ++++++++++------------------
> dbus/dbus-sysdeps.h          | 13 ++++++------
> dbus/dbus-transport-socket.c | 47 ++++++++++++++++++++++++++++++--------------
> 7 files changed, 90 insertions(+), 51 deletions(-)
>
>diff --git a/dbus/dbus-nonce.c b/dbus/dbus-nonce.c
>index 37f30f0..44c46b2 100644
>--- a/dbus/dbus-nonce.c
>+++ b/dbus/dbus-nonce.c
>@@ -53,10 +53,14 @@ do_check_nonce (int fd, const DBusString *nonce, DBusError *error)
> 
>   while (nleft)
>     {
>+      int saved_errno;
>+
>       n = _dbus_read_socket (fd, &p, nleft);
>-      if (n == -1 && _dbus_get_is_errno_eintr())
>+      saved_errno = _dbus_save_socket_errno ();
>+
>+      if (n == -1 && _dbus_get_is_errno_eintr (saved_errno))
>         ;
>-      else if (n == -1 && _dbus_get_is_errno_eagain_or_ewouldblock())
>+      else if (n == -1 && _dbus_get_is_errno_eagain_or_ewouldblock (saved_errno))
>         _dbus_sleep_milliseconds (100);
>       else if (n==-1)
>         {
>diff --git a/dbus/dbus-server-socket.c b/dbus/dbus-server-socket.c
>index 060a919..70367c7 100644
>--- a/dbus/dbus-server-socket.c
>+++ b/dbus/dbus-server-socket.c
>@@ -184,6 +184,7 @@ socket_handle_watch (DBusWatch    *watch,
>     {
>       int client_fd;
>       int listen_fd;
>+      int saved_errno;
> 
>       listen_fd = dbus_watch_get_socket (watch);
> 
>@@ -192,15 +193,17 @@ socket_handle_watch (DBusWatch    *watch,
>       else 
>           client_fd = _dbus_accept (listen_fd);
> 
>+      saved_errno = _dbus_save_socket_errno ();
>+
>       if (client_fd < 0)
>         {
>           /* EINTR handled for us */
> 
>-          if (_dbus_get_is_errno_eagain_or_ewouldblock ())
>+          if (_dbus_get_is_errno_eagain_or_ewouldblock (saved_errno))
>             _dbus_verbose ("No client available to accept after all\n");
>           else
>             _dbus_verbose ("Failed to accept a client connection: %s\n",
>-                           _dbus_strerror_from_errno ());
>+                           _dbus_strerror (saved_errno));
> 
>           SERVER_UNLOCK (server);
>         }
>diff --git a/dbus/dbus-sysdeps-unix.c b/dbus/dbus-sysdeps-unix.c
>index 1b52375..0e9f80e 100644
>--- a/dbus/dbus-sysdeps-unix.c
>+++ b/dbus/dbus-sysdeps-unix.c
>@@ -3926,12 +3926,12 @@ _dbus_daemon_unpublish_session_bus_address (void)
>  * See if errno is EAGAIN or EWOULDBLOCK (this has to be done differently
>  * for Winsock so is abstracted)
>  *
>- * @returns #TRUE if errno == EAGAIN or errno == EWOULDBLOCK
>+ * @returns #TRUE if e == EAGAIN or e == EWOULDBLOCK
>  */
> dbus_bool_t
>-_dbus_get_is_errno_eagain_or_ewouldblock (void)
>+_dbus_get_is_errno_eagain_or_ewouldblock (int e)
> {
>-  return errno == EAGAIN || errno == EWOULDBLOCK;
>+  return e == EAGAIN || e == EWOULDBLOCK;
> }
> 
> /**
>@@ -4188,4 +4188,16 @@ _dbus_append_address_from_socket (int         fd,
>   return FALSE;
> }
> 
>+int
>+_dbus_save_socket_errno (void)
>+{
>+  return errno;
>+}
>+
>+void
>+_dbus_restore_socket_errno (int saved_errno)
>+{
>+  errno = saved_errno;
>+}
>+
> /* tests in dbus-sysdeps-util.c */
>diff --git a/dbus/dbus-sysdeps-win.c b/dbus/dbus-sysdeps-win.c
>index 1167e96..7cf783b 100644
>--- a/dbus/dbus-sysdeps-win.c
>+++ b/dbus/dbus-sysdeps-win.c
>@@ -3252,12 +3252,12 @@ _dbus_flush_caches (void)
>  * See if errno is EAGAIN or EWOULDBLOCK (this has to be done differently
>  * for Winsock so is abstracted)
>  *
>- * @returns #TRUE if errno == EAGAIN or errno == EWOULDBLOCK
>+ * @returns #TRUE if e == EAGAIN or e == EWOULDBLOCK
>  */
> dbus_bool_t
>-_dbus_get_is_errno_eagain_or_ewouldblock (void)
>+_dbus_get_is_errno_eagain_or_ewouldblock (int e)
> {
>-  return errno == WSAEWOULDBLOCK;
>+  return e == WSAEWOULDBLOCK;
> }
> 
> /**
>@@ -3724,6 +3724,18 @@ _dbus_check_setuid (void)
>   return FALSE;
> }
> 
>+int
>+_dbus_save_socket_errno (void)
>+{
>+  return errno;
>+}
>+
>+void
>+_dbus_restore_socket_errno (int saved_errno)
>+{
>+  _dbus_win_set_errno (saved_errno);
>+}
>+
> /** @} end of sysdeps-win */
> /* tests in dbus-sysdeps-util.c */
> 
>diff --git a/dbus/dbus-sysdeps.c b/dbus/dbus-sysdeps.c
>index f4ba0fa..9979210 100644
>--- a/dbus/dbus-sysdeps.c
>+++ b/dbus/dbus-sysdeps.c
>@@ -722,33 +722,23 @@ _dbus_set_errno_to_zero (void)
> }
> 
> /**
>- * See if errno is set
>- * @returns #TRUE if errno is not 0
>- */
>-dbus_bool_t
>-_dbus_get_is_errno_nonzero (void)
>-{
>-  return errno != 0;
>-}
>-
>-/**
>  * See if errno is ENOMEM
>- * @returns #TRUE if errno == ENOMEM
>+ * @returns #TRUE if e == ENOMEM
>  */
> dbus_bool_t
>-_dbus_get_is_errno_enomem (void)
>+_dbus_get_is_errno_enomem (int e)
> {
>-  return errno == ENOMEM;
>+  return e == ENOMEM;
> }
> 
> /**
>  * See if errno is EINTR
>- * @returns #TRUE if errno == EINTR
>+ * @returns #TRUE if e == EINTR
>  */
> dbus_bool_t
>-_dbus_get_is_errno_eintr (void)
>+_dbus_get_is_errno_eintr (int e)
> {
>-  return errno == EINTR;
>+  return e == EINTR;
> }
> 
> /**
>@@ -756,9 +746,9 @@ _dbus_get_is_errno_eintr (void)
>  * @returns #TRUE if errno == EPIPE
>  */
> dbus_bool_t
>-_dbus_get_is_errno_epipe (void)
>+_dbus_get_is_errno_epipe (int e)
> {
>-  return errno == EPIPE;
>+  return e == EPIPE;
> }
> 
> /**
>@@ -766,10 +756,10 @@ _dbus_get_is_errno_epipe (void)
>  * @returns #TRUE if errno == ETOOMANYREFS
>  */
> dbus_bool_t
>-_dbus_get_is_errno_etoomanyrefs (void)
>+_dbus_get_is_errno_etoomanyrefs (int e)
> {
> #ifdef ETOOMANYREFS
>-  return errno == ETOOMANYREFS;
>+  return e == ETOOMANYREFS;
> #else
>   return FALSE;
> #endif
>diff --git a/dbus/dbus-sysdeps.h b/dbus/dbus-sysdeps.h
>index 21033eb..41c317c 100644
>--- a/dbus/dbus-sysdeps.h
>+++ b/dbus/dbus-sysdeps.h
>@@ -378,13 +378,14 @@ dbus_bool_t _dbus_generate_random_ascii        (DBusString *str,
> const char* _dbus_error_from_errno (int error_number);
> const char* _dbus_error_from_system_errno (void);
> 
>+int         _dbus_save_socket_errno                  (void);
>+void        _dbus_restore_socket_errno               (int saved_errno);
> void        _dbus_set_errno_to_zero                  (void);
>-dbus_bool_t _dbus_get_is_errno_nonzero               (void);
>-dbus_bool_t _dbus_get_is_errno_eagain_or_ewouldblock (void);
>-dbus_bool_t _dbus_get_is_errno_enomem                (void);
>-dbus_bool_t _dbus_get_is_errno_eintr                 (void);
>-dbus_bool_t _dbus_get_is_errno_epipe                 (void);
>-dbus_bool_t _dbus_get_is_errno_etoomanyrefs           (void);
>+dbus_bool_t _dbus_get_is_errno_eagain_or_ewouldblock (int e);
>+dbus_bool_t _dbus_get_is_errno_enomem                (int e);
>+dbus_bool_t _dbus_get_is_errno_eintr                 (int e);
>+dbus_bool_t _dbus_get_is_errno_epipe                 (int e);
>+dbus_bool_t _dbus_get_is_errno_etoomanyrefs          (int e);
> const char* _dbus_strerror_from_errno                (void);
> 
> void _dbus_disable_sigpipe (void);
>diff --git a/dbus/dbus-transport-socket.c b/dbus/dbus-transport-socket.c
>index 199d3b5..c1e4701 100644
>--- a/dbus/dbus-transport-socket.c
>+++ b/dbus/dbus-transport-socket.c
>@@ -247,13 +247,15 @@ read_data_into_auth (DBusTransport *transport,
>   DBusTransportSocket *socket_transport = (DBusTransportSocket*) transport;
>   DBusString *buffer;
>   int bytes_read;
>-  
>+  int saved_errno;
>+
>   *oom = FALSE;
> 
>   _dbus_auth_get_buffer (transport->auth, &buffer);
>   
>   bytes_read = _dbus_read_socket (socket_transport->fd,
>                                   buffer, socket_transport->max_bytes_read_per_iteration);
>+  saved_errno = _dbus_save_socket_errno ();
> 
>   _dbus_auth_return_buffer (transport->auth, buffer);
> 
>@@ -267,16 +269,16 @@ read_data_into_auth (DBusTransport *transport,
>     {
>       /* EINTR already handled for us */
> 
>-      if (_dbus_get_is_errno_enomem ())
>+      if (_dbus_get_is_errno_enomem (saved_errno))
>         {
>           *oom = TRUE;
>         }
>-      else if (_dbus_get_is_errno_eagain_or_ewouldblock ())
>+      else if (_dbus_get_is_errno_eagain_or_ewouldblock (saved_errno))
>         ; /* do nothing, just return FALSE below */
>       else
>         {
>           _dbus_verbose ("Error reading from remote app: %s\n",
>-                         _dbus_strerror_from_errno ());
>+                         _dbus_strerror (saved_errno));
>           do_io_error (transport);
>         }
> 
>@@ -299,6 +301,7 @@ write_data_from_auth (DBusTransport *transport)
> {
>   DBusTransportSocket *socket_transport = (DBusTransportSocket*) transport;
>   int bytes_written;
>+  int saved_errno;
>   const DBusString *buffer;
> 
>   if (!_dbus_auth_get_bytes_to_send (transport->auth,
>@@ -308,6 +311,7 @@ write_data_from_auth (DBusTransport *transport)
>   bytes_written = _dbus_write_socket (socket_transport->fd,
>                                       buffer,
>                                       0, _dbus_string_get_length (buffer));
>+  saved_errno = _dbus_save_socket_errno ();
> 
>   if (bytes_written > 0)
>     {
>@@ -318,12 +322,12 @@ write_data_from_auth (DBusTransport *transport)
>     {
>       /* EINTR already handled for us */
>       
>-      if (_dbus_get_is_errno_eagain_or_ewouldblock ())
>+      if (_dbus_get_is_errno_eagain_or_ewouldblock (saved_errno))
>         ;
>       else
>         {
>           _dbus_verbose ("Error writing to remote app: %s\n",
>-                         _dbus_strerror_from_errno ());
>+                         _dbus_strerror (saved_errno));
>           do_io_error (transport);
>         }
>     }
>@@ -527,6 +531,7 @@ do_writing (DBusTransport *transport)
>       const DBusString *body;
>       int header_len, body_len;
>       int total_bytes_to_write;
>+      int saved_errno;
>       
>       if (total > socket_transport->max_bytes_written_per_iteration)
>         {
>@@ -584,6 +589,7 @@ do_writing (DBusTransport *transport)
>                                 &socket_transport->encoded_outgoing,
>                                 socket_transport->message_bytes_written,
>                                 total_bytes_to_write - socket_transport->message_bytes_written);
>+          saved_errno = _dbus_save_socket_errno ();
>         }
>       else
>         {
>@@ -612,6 +618,7 @@ do_writing (DBusTransport *transport)
>                                                       0, body_len,
>                                                       unix_fds,
>                                                       n);
>+              saved_errno = _dbus_save_socket_errno ();
> 
>               if (bytes_written > 0 && n > 0)
>                 _dbus_verbose("Wrote %i unix fds\n", n);
>@@ -638,6 +645,8 @@ do_writing (DBusTransport *transport)
>                                         body_len -
>                                         (socket_transport->message_bytes_written - header_len));
>                 }
>+
>+              saved_errno = _dbus_save_socket_errno ();
>             }
>         }
> 
>@@ -651,7 +660,7 @@ do_writing (DBusTransport *transport)
>            * http://lists.freedesktop.org/archives/dbus/2008-March/009526.html
>            */
>           
>-          if (_dbus_get_is_errno_eagain_or_ewouldblock () || _dbus_get_is_errno_epipe ())
>+          if (_dbus_get_is_errno_eagain_or_ewouldblock (saved_errno) || _dbus_get_is_errno_epipe (saved_errno))
>             goto out;
> 
>           /* Since Linux commit 25888e (from 2.6.37-rc4, Nov 2010), sendmsg()
>@@ -663,7 +672,7 @@ do_writing (DBusTransport *transport)
>            * https://bugs.freedesktop.org/show_bug.cgi?id=80163
>            */
> 
>-          else if (_dbus_get_is_errno_etoomanyrefs ())
>+          else if (_dbus_get_is_errno_etoomanyrefs (saved_errno))
>             {
>               /* We only send fds in the first byte of the message.
>                * ETOOMANYREFS cannot happen after.
>@@ -686,7 +695,7 @@ do_writing (DBusTransport *transport)
>           else
>             {
>               _dbus_verbose ("Error writing to remote app: %s\n",
>-                             _dbus_strerror_from_errno ());
>+                             _dbus_strerror (saved_errno));
>               do_io_error (transport);
>               goto out;
>             }
>@@ -730,6 +739,7 @@ do_reading (DBusTransport *transport)
>   int bytes_read;
>   int total;
>   dbus_bool_t oom;
>+  int saved_errno;
> 
>   _dbus_verbose ("fd = %d\n",socket_transport->fd);
>   
>@@ -774,6 +784,8 @@ do_reading (DBusTransport *transport)
>                                         &socket_transport->encoded_incoming,
>                                         socket_transport->max_bytes_read_per_iteration);
> 
>+      saved_errno = _dbus_save_socket_errno ();
>+
>       _dbus_assert (_dbus_string_get_length (&socket_transport->encoded_incoming) ==
>                     bytes_read);
>       
>@@ -823,6 +835,7 @@ do_reading (DBusTransport *transport)
>                                                        buffer,
>                                                        socket_transport->max_bytes_read_per_iteration,
>                                                        fds, &n_fds);
>+          saved_errno = _dbus_save_socket_errno ();
> 
>           if (bytes_read >= 0 && n_fds > 0)
>             _dbus_verbose("Read %i unix fds\n", n_fds);
>@@ -834,28 +847,29 @@ do_reading (DBusTransport *transport)
>         {
>           bytes_read = _dbus_read_socket (socket_transport->fd,
>                                           buffer, socket_transport->max_bytes_read_per_iteration);
>+          saved_errno = _dbus_save_socket_errno ();
>         }
> 
>       _dbus_message_loader_return_buffer (transport->loader,
>                                           buffer);
>     }
>-  
>+
>   if (bytes_read < 0)
>     {
>       /* EINTR already handled for us */
> 
>-      if (_dbus_get_is_errno_enomem ())
>+      if (_dbus_get_is_errno_enomem (saved_errno))
>         {
>           _dbus_verbose ("Out of memory in read()/do_reading()\n");
>           oom = TRUE;
>           goto out;
>         }
>-      else if (_dbus_get_is_errno_eagain_or_ewouldblock ())
>+      else if (_dbus_get_is_errno_eagain_or_ewouldblock (saved_errno))
>         goto out;
>       else
>         {
>           _dbus_verbose ("Error reading from remote app: %s\n",
>-                         _dbus_strerror_from_errno ());
>+                         _dbus_strerror (saved_errno));
>           do_io_error (transport);
>           goto out;
>         }
>@@ -1129,6 +1143,8 @@ socket_do_iteration (DBusTransport *transport,
> 
>   if (poll_fd.events)
>     {
>+      int saved_errno;
>+
>       if (flags & DBUS_ITERATION_BLOCK)
> 	poll_timeout = timeout_milliseconds;
>       else
>@@ -1147,8 +1163,9 @@ socket_do_iteration (DBusTransport *transport,
>       
>     again:
>       poll_res = _dbus_poll (&poll_fd, 1, poll_timeout);
>+      saved_errno = _dbus_save_socket_errno ();
> 
>-      if (poll_res < 0 && _dbus_get_is_errno_eintr ())
>+      if (poll_res < 0 && _dbus_get_is_errno_eintr (saved_errno))
> 	goto again;
> 
>       if (flags & DBUS_ITERATION_BLOCK)
>@@ -1191,7 +1208,7 @@ socket_do_iteration (DBusTransport *transport,
>       else
>         {
>           _dbus_verbose ("Error from _dbus_poll(): %s\n",
>-                         _dbus_strerror_from_errno ());
>+                         _dbus_strerror (saved_errno));
>         }
>     }
> 
>-- 
>2.1.0
>

Actions: View | Splinter Review

Attachments on bug 83625: 105916

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.