From a35281a0e8570b94ee36023d45dac0fd53a15c77 Mon Sep 17 00:00:00 2001 From: Simon McVittie 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