Bug 69073 - [PATCH] fix/improve SOCK_CLOEXEC handling when defined but not supported by socket/socketpair
Summary: [PATCH] fix/improve SOCK_CLOEXEC handling when defined but not supported by s...
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.4.x
Hardware: All other
: medium normal
Assignee: Havoc Pennington
QA Contact:
URL:
Whiteboard: review+
Keywords: patch
Depends on:
Blocks:
 
Reported: 2013-09-07 15:31 UTC by Pino Toscano
Modified: 2013-09-13 13:52 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:

Attachments
Check also for EPROTOTYPE (1.39 KB, text/plain)
2013-09-07 15:31 UTC, Pino Toscano 		Details
[PATCH 1/2] Check EINVAL for socketpair and retry without SOCK_CLOEXEC (1.78 KB, patch)
2013-09-10 14:47 UTC, Chengwei Yang 		Details | Splinter Review
[PATCH 2/2] Allow EPROTOTYPE for SOCK_CLOEXEC but unsupported by socket/socketpair (1.73 KB, patch)
2013-09-10 14:48 UTC, Chengwei Yang 		Details | Splinter Review
Show Obsolete (1) View All

Description Pino Toscano 2013-09-07 15:31:59 UTC 
Created attachment 85398 [details]
Check also for EPROTOTYPE

_dbus_open_socket and _dbus_full_duplex_pipe in dbus-sysdeps-unix.c can make use of SOCK_CLOEXEC if defined, trying to use it and eventually setting the cloexec bit if not succeeded at creation time.
Although, if SOCK_CLOEXEC is defined (most probably because accept4 is implemented) but not supported by socket and socketpair yet, only EINVAL (the return value on Linux) is checked, while the canonical POSIX errno value for an invalid socket type is EPROTOTYPE.
The above is what happens on GNU/Hurd after accept4 (and thus SOCK_* flags) was added in glibc.

Attached there is a small commit (based on the dbus-1.6 branch) which checks for EPROTOTYPE in addition to EINVAL.
(Most probably also _dbus_connect_exec should have a similar runtime check wrt the socketpair+SOCK_CLOEXEC failure as _dbus_open_socket and _dbus_full_duplex_pipe do, but that could be done later if needed, I guess.)
Comment 1 Chengwei Yang 2013-09-10 14:45:13 UTC 
Comment on attachment 85398 [details]
Check also for EPROTOTYPE

Pino Toscano, I'll upload a V2 of your patch with the same check for _dbus_connect_exec() based on a fix for _dbus_connect_exec().
Comment 2 Chengwei Yang 2013-09-10 14:47:22 UTC 
Created attachment 85564 [details] [review]
[PATCH 1/2] Check EINVAL for socketpair and retry without  SOCK_CLOEXEC
Comment 3 Chengwei Yang 2013-09-10 14:48:01 UTC 
Created attachment 85565 [details] [review]
[PATCH 2/2] Allow EPROTOTYPE for SOCK_CLOEXEC but unsupported by  socket/socketpair
Comment 4 Pino Toscano 2013-09-10 15:15:38 UTC 
(In reply to comment #1)
> Pino Toscano, I'll upload a V2 of your patch with the same check for
> _dbus_connect_exec() based on a fix for _dbus_connect_exec().

Your patch and the updated version of mine look okay, thanks!
Comment 5 Simon McVittie 2013-09-13 12:31:46 UTC 
Comment on attachment 85564 [details] [review]
[PATCH 1/2] Check EINVAL for socketpair and retry without  SOCK_CLOEXEC

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

::: dbus/dbus-sysdeps-unix.c
@@ +902,2 @@
>  #ifdef SOCK_CLOEXEC
> +  retval = socketpair(AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0, fds);

socketpair (

(with the space)

@@ +902,3 @@
>  #ifdef SOCK_CLOEXEC
> +  retval = socketpair(AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0, fds);
> +  cloexec_done = retval >= 0;

I'd prefer cloexec_done = (retval >= 0) to make the precedence obvious.

@@ +907,3 @@
>  #endif
> +    {
> +      retval = socketpair(AF_UNIX, SOCK_STREAM, 0, fds);

with a space again
Comment 6 Simon McVittie 2013-09-13 12:36:22 UTC 
Applied to my local branch with trivial coding style fixes, will merge after testing
Comment 7 Simon McVittie 2013-09-13 13:52:50 UTC 
Fixed in git for 1.6.16, 1.7.6. Thanks!

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.