Bug 106395

Summary:	should not assume that getaddrinfo sets errno
Product:	dbus	Reporter:	Simon McVittie <smcv>
Component:	core	Assignee:	Simon McVittie <smcv>
Status:	RESOLVED FIXED	QA Contact:	D-Bus Maintainers <dbus>
Severity:	normal
Priority:	medium	Keywords:	patch
Version:	git master
Hardware:	Other
OS:	All
Whiteboard:	review+
i915 platform:		i915 features:
Bug Depends on:
Bug Blocks:	106812
Attachments:	sysdeps-unix: Handle errors from getaddrinfo correctly

Description Simon McVittie 2018-05-03 22:39:56 UTC

dbus-sysdeps-unix assumes that getaddrinfo() will always set errno, and uses _dbus_error_from_errno (errno) afterwards.

However, getaddrinfo() actually only guarantees to set errno if it returns EAI_SYSTEM. If it returns a different nonzero EAI_* error code, then errno is left set to whatever happens to have gone wrong most recently - perhaps nothing to do with the actual error we saw!

Linux getaddrinfo(3) documents:

EAI_ADDRFAMILY: host has no network addresses in that family
(e.g. we asked for IPv6 but it only has IPv4)

EAI_AGAIN: temporary failure

EAI_BADFLAGS: programming error in setting hints.ai_flags
(~= EINVAL)

EAI_FAMILY: address family is not supported (at all)

EAI_MEMORY: out of memory (~= ENOMEM)

EAI_NODATA: host exists but has no addresses

EAI_NONAME: not found

EAI_SERVICE: service not available for specified socket type
(we should never see this)

EAI_SOCKTYPE: requested socket type unsupported

EAI_SYSTEM: see errno

Minimal correct handling is that we should use _dbus_error_from_errno() for EAI_SYSTEM and DBUS_ERROR_FAILED for the rest. Slightly better would be to use DBUS_ERROR_NO_MEMORY for EAI_MEMORY. DBUS_ERROR_NOT_SUPPORTED might be more appropriate than FAILED for EAI_FAMILY, and possibly others.

Optionally, we could invent more specific error names, but given that we generally discourage TCP on Unix, there's probably little point - these errors mostly only show up in unit tests where we happen to be using TCP for the sake of running the same code on Unix and Windows.

(getaddrinfo() on Windows returns a Winsock error code, which we seem to be handling correctly, so for once there's nothing to be fixed in dbus-sysdeps-win.)

Comment 1 Simon McVittie 2018-06-04 15:35:10 UTC

Created attachment 140010 [details] [review]
sysdeps-unix: Handle errors from getaddrinfo correctly

getaddrinfo and getnameinfo have their own error-handling convention
in which the library call returns either 0 or an EAI_* error code
unrelated to errno. If the error code is not EAI_SYSTEM, then
the value of errno is undefined (in particular it might be carried
over from a previous system call or library call). Introduce a
new helper function _dbus_error_from_gai() to handle this.

The equivalent code paths in Windows appear to be OK: the Windows
implementation of getaddrinfo() is documented to return a Winsock
error code, which we seem to be handling correctly.

Comment 2 Simon McVittie 2018-06-04 15:46:04 UTC

Comment on attachment 140010 [details] [review]
sysdeps-unix: Handle errors from getaddrinfo correctly

Review of attachment 140010 [details] [review]:
-----------------------------------------------------------------

::: dbus/dbus-sysdeps-unix.c
@@ +1327,5 @@
>  
> +/* Convert an error code from getaddrinfo() or getnameinfo() into
> + * a D-Bus error name. */
> +static const char *
> +_dbus_error_from_gai (int gai_res,

I chose this naming because gai seems to be a conventional name for the RFC 2553/RFC 3493 family of functions (they use gai_strerror() and, in glibc, are configurable via gai.conf).

Comment 3 Philip Withnall 2018-06-04 16:06:58 UTC

Comment on attachment 140010 [details] [review]
sysdeps-unix: Handle errors from getaddrinfo correctly

Review of attachment 140010 [details] [review]:
-----------------------------------------------------------------

r+

Comment 4 Simon McVittie 2018-06-04 17:00:04 UTC

Fixed in git for 1.13.6, and I'm testing cherry-picked versions for 1.12.10. Thanks for the reviews!

Comment 5 Ralf Habacker 2018-06-05 07:18:20 UTC

(In reply to Simon McVittie from comment #1)
> The equivalent code paths in Windows appear to be OK: the Windows
> implementation of getaddrinfo() is documented to return a Winsock
> error code, which we seem to be handling correctly.

Thanks for looking into the Windows part.

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.