From patchwork Wed Nov 5 07:57:26 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alexandre Oliva X-Patchwork-Id: 3586 Received: (qmail 19290 invoked by alias); 5 Nov 2014 07:59:33 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 19279 invoked by uid 89); 5 Nov 2014 07:59:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.3 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS, T_TVD_MIME_NO_HEADERS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com From: Alexandre Oliva To: libc-alpha@sourceware.org Subject: RFC on enforcing best-practice through wrappers? [PR15819, PR15722] Date: Wed, 05 Nov 2014 05:57:26 -0200 Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 These two PRs are about unrelated issues, but they share a common theme: standard APIs do not follow best practice by default, our code often fails to jump through the required hoops to do so and, even if it did, it would be best to factor out what it takes to follow best practice into a single place, and use it all over, perhaps even by default. The two cases at hand are PR15819 (poll EINTRs and we retry without adjusting the timeout) and PR15722 (sockets are created without CLOEXEC). There are various ways to go about fixing this, ranging from using __poll and __socket internal wrappers/aliases to avoid the undesirable problems in the first place (say, having __poll never return EINTR, but retrying with a recomputed timeout instead instead, and having __socket always set CLOEXEC), introducing alternate functions to do so and perhaps poisoning the original symbols so that misuses are flagged, up to just offering wrappers that implement the best practice in internal header wrappers or new internal headers. AFAICT we don't seem to have any recommended practice on how to factor out, let alone internally enforce, this sort of best practice. In these two patches, I've followed the last two approaches: one introduces a static inline wrapper __poll_noeintr in sys/poll.h, that will reenter the syscall with a recomputed timeout if it's interrupted, and the other introduces a new header socket-cloexec.h that defines __socket_cloexec and __socket_cloexec_nonblock static inline wrappers to __socket that do what it takes to set these flags even when SOCK_CLOEXEC and SOCK_NONBLOCK are not supported. The reason for the differences is that I'd arranged for the headers to include all headers required for functions using errno, gettimeofday, fcntl, ioctl and whatnot to be properly parsed, and in the case of __socket_cloexec, this became an include loop that caused some headers to fail to be parsed correctly because referenced types hadn't been defined after the nested #include of the header supposed to define them. I didn't see much precedent for static inline functions in internal headers, so I'm a bit hesitant to go either way and maybe set an unintended precedent. OTOH, using preprocessor macros, which would alleviate the problem of #include loops because the macros would only be parsed at expansion time, is more fragile to begin with, and there is still a risk that mutually-dependent headers get included in the wrong order and break stuff. OYAH (On yet another hand ;-) using out-of-line functions would introduce another layer of indirection between the syscall and its client, which might be undesirable from a performance perspective. Any thoughts or recommendations on how we should address this sort of issue, ideally in a way that makes it easier to follow best practice than to forget to do so? While at that, are these interfaces ones we might want to offer to our users, as GNU extensions? Is it overkill to offer the __socket_cloexec extra argument, on whether or not CLOEXEC is mandatory and the whole thing should fail if we can't make the socket cloexec? Should __socket_cloexec_nonblock attempt a third way to make a socket non-blocking, if the first two fail? (absent SOCK_NONBLOCK, I'm trying fcntl F_SETFL, like nscd, but I see sunrpc/clnt_udp.c uses the FIONBIO ioctl) Thanks in advance, Here are patchlets that implement two of the alternatives I've described, for reference. Once we agree on one standard way to do this sort of thing, I'll adjust the patches as needed and submit them in a form more suitable for inclusion. create all sockets with SOCK_CLOEXEC From: Alexandre Oliva --- include/socket-cloexec.h | 105 ++++++++++++++++++++++++++++++++ resolv/res_hconf.c | 3 + socket/opensock.c | 15 ++--- sunrpc/clnt_tcp.c | 3 + sunrpc/clnt_unix.c | 3 + sunrpc/pm_getport.c | 3 + sunrpc/pmap_rmt.c | 3 + sunrpc/rtime.c | 3 + sunrpc/svc_tcp.c | 4 + sunrpc/svc_udp.c | 4 + sunrpc/svc_unix.c | 3 + sysdeps/gnu/ifaddrs.c | 3 + sysdeps/posix/getaddrinfo.c | 3 + sysdeps/unix/sysv/linux/check_native.c | 4 + sysdeps/unix/sysv/linux/check_pf.c | 3 + sysdeps/unix/sysv/linux/ifaddrs.c | 3 + 16 files changed, 144 insertions(+), 21 deletions(-) create mode 100644 include/socket-cloexec.h diff --git a/include/socket-cloexec.h b/include/socket-cloexec.h new file mode 100644 index 0000000..8ada604 --- /dev/null +++ b/include/socket-cloexec.h @@ -0,0 +1,105 @@ +/* Copyright (C) 2014 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + . */ + +#ifndef _SOCKET_CLOEXEC_H +#define _SOCKET_CLOEXEC_H + +#include +#include +#include +#include +#include + +/* Like socket, but with SOCK_CLOEXEC set if available. If it's not, + try to set the FD_CLOEXEC flag, and if that fails, close the socket + and fail unless __tolerant. */ +static inline int +__socket_cloexec (int __domain, int __type, int __protocol, bool __tolerant) +{ + int __ret; +#ifdef SOCK_CLOEXEC +# ifdef __ASSUME_SOCK_CLOEXEC + int __have_sock_cloexec = 1; +# endif /* __ASSUME_SOCK_CLOEXEC */ + if (__have_sock_cloexec >= 0) + { + __ret = __socket (__domain, __type | SOCK_CLOEXEC, __protocol); + if (__have_sock_cloexec == 0) + __have_sock_cloexec = (__ret == -1 && errno == EINVAL ? -1 : 1); + if (__have_sock_cloexec > 0) + return __ret; + } +#endif /* SOCK_CLOEXEC */ + __ret = __socket (__domain, __type, __protocol); + + if ((__ret >= 0 && __fcntl (__ret, F_SETFD, FD_CLOEXEC) < 0) && !__tolerant) + { + __close (__ret); + __ret = -1; + } + + return __ret; +} + +/* Like socket, but with SOCK_CLOEXEC and SOCK_NONBLOCK set if + SOCK_CLOEXEC is available. SOCK_NONBLOCK is supported iff + SOCK_CLOEXEC is. + + If SOCK_CLOEXEC is not supported, try to set the FD_CLOEXEC flag, + and if that fails, close the socket and fail unless __tolerant. + + If SOCK_NONBLOCK is not supported, try to set the O_NONBLOCK flag, + and if that fails, close the socket and fail REGARDLESS of + __tolerant. */ +static inline int +__socket_cloexec_nonblock (int __domain, int __type, int __protocol, + bool __tolerant) +{ + int __ret; +#ifdef SOCK_NONBLOCK +# ifdef __ASSUME_SOCK_CLOEXEC + int __have_sock_cloexec = 1; +# endif /* __ASSUME_SOCK_CLOEXEC */ + if (__have_sock_cloexec >= 0) + { + __ret = __socket (__domain, __type | SOCK_CLOEXEC | SOCK_NONBLOCK, + __protocol); + if (__have_sock_cloexec == 0) + __have_sock_cloexec = (__ret == -1 && errno == EINVAL ? -1 : 1); + if (__have_sock_cloexec > 0) + return __ret; + } +#endif /* SOCK_NONBLOCK */ + __ret = __socket (__domain, __type, __protocol); + if (__ret >= 0) + { + int __fl = __fcntl (__ret, F_GETFL); + if (__fl == -1 + || __fcntl (__ret, F_SETFL, __fl | O_NONBLOCK) < 0 + || (__fcntl (__ret, F_SETFD, FD_CLOEXEC) < 0 && !__tolerant)) + { + __close (__ret); + __ret = -1; + } + } + return __ret; +} + +#pragma poison __socket +#pragma poison socket + +#endif /* _SOCKET_CLOEXEC_H */ diff --git a/resolv/res_hconf.c b/resolv/res_hconf.c index b4c86227..1df336c 100644 --- a/resolv/res_hconf.c +++ b/resolv/res_hconf.c @@ -41,6 +41,7 @@ #include #include #include +#include #include #include "ifreq.h" #include "res_hconf.h" @@ -410,7 +411,7 @@ _res_hconf_reorder_addrs (struct hostent *hp) /* Initialize interface table. */ /* The SIOCGIFNETMASK ioctl will only work on an AF_INET socket. */ - sd = __socket (AF_INET, SOCK_DGRAM, 0); + sd = __socket_cloexec (AF_INET, SOCK_DGRAM, 0, true); if (sd < 0) return; diff --git a/socket/opensock.c b/socket/opensock.c index 8dd8906..d23a962 100644 --- a/socket/opensock.c +++ b/socket/opensock.c @@ -17,6 +17,7 @@ #include #include +#include #include /* Return a socket of any type. The socket can be used in subsequent @@ -32,7 +33,7 @@ __opensock (void) if (sock_af != -1) { - fd = __socket (sock_af, SOCK_DGRAM, 0); + fd = __socket_cloexec (sock_af, SOCK_DGRAM, 0, true); if (fd != -1) return fd; } @@ -40,28 +41,28 @@ __opensock (void) __libc_lock_lock (lock); if (sock_af != -1) - fd = __socket (sock_af, SOCK_DGRAM, 0); + fd = __socket_cloexec (sock_af, SOCK_DGRAM, 0, true); if (fd == -1) { #ifdef AF_INET - fd = __socket (sock_af = AF_INET, SOCK_DGRAM, 0); + fd = __socket_cloexec (sock_af = AF_INET, SOCK_DGRAM, 0, true); #endif #ifdef AF_INET6 if (fd < 0) - fd = __socket (sock_af = AF_INET6, SOCK_DGRAM, 0); + fd = __socket_cloexec (sock_af = AF_INET6, SOCK_DGRAM, 0, true); #endif #ifdef AF_IPX if (fd < 0) - fd = __socket (sock_af = AF_IPX, SOCK_DGRAM, 0); + fd = __socket_cloexec (sock_af = AF_IPX, SOCK_DGRAM, 0, true); #endif #ifdef AF_AX25 if (fd < 0) - fd = __socket (sock_af = AF_AX25, SOCK_DGRAM, 0); + fd = __socket_cloexec (sock_af = AF_AX25, SOCK_DGRAM, 0, true); #endif #ifdef AF_APPLETALK if (fd < 0) - fd = __socket (sock_af = AF_APPLETALK, SOCK_DGRAM, 0); + fd = __socket_cloexec (sock_af = AF_APPLETALK, SOCK_DGRAM, 0, true); #endif } diff --git a/sunrpc/clnt_tcp.c b/sunrpc/clnt_tcp.c index f4ba0ef..2dd07cf 100644 --- a/sunrpc/clnt_tcp.c +++ b/sunrpc/clnt_tcp.c @@ -52,6 +52,7 @@ #include #include #include +#include #include #include @@ -146,7 +147,7 @@ clnttcp_create (struct sockaddr_in *raddr, u_long prog, u_long vers, */ if (*sockp < 0) { - *sockp = __socket (AF_INET, SOCK_STREAM, IPPROTO_TCP); + *sockp = __socket_cloexec (AF_INET, SOCK_STREAM, IPPROTO_TCP, true); (void) bindresvport (*sockp, (struct sockaddr_in *) 0); if ((*sockp < 0) || (__connect (*sockp, (struct sockaddr *) raddr, diff --git a/sunrpc/clnt_unix.c b/sunrpc/clnt_unix.c index aff6fa5..419a58a 100644 --- a/sunrpc/clnt_unix.c +++ b/sunrpc/clnt_unix.c @@ -53,6 +53,7 @@ #include #include #include +#include #include #include @@ -132,7 +133,7 @@ clntunix_create (struct sockaddr_un *raddr, u_long prog, u_long vers, */ if (*sockp < 0) { - *sockp = __socket (AF_UNIX, SOCK_STREAM, 0); + *sockp = __socket_cloexec (AF_UNIX, SOCK_STREAM, 0, true); len = strlen (raddr->sun_path) + sizeof (raddr->sun_family) + 1; if (*sockp < 0 || __connect (*sockp, (struct sockaddr *) raddr, len) < 0) diff --git a/sunrpc/pm_getport.c b/sunrpc/pm_getport.c index d9df0cc..31f8dcc 100644 --- a/sunrpc/pm_getport.c +++ b/sunrpc/pm_getport.c @@ -38,6 +38,7 @@ #include #include #include +#include /* * Create a socket that is locally bound to a non-reserve port. For @@ -48,7 +49,7 @@ int internal_function __get_socket (struct sockaddr_in *saddr) { - int so = __socket (AF_INET, SOCK_STREAM, IPPROTO_TCP); + int so = __socket_cloexec (AF_INET, SOCK_STREAM, IPPROTO_TCP, true); if (so < 0) return -1; diff --git a/sunrpc/pmap_rmt.c b/sunrpc/pmap_rmt.c index fd8de85..389eb9e 100644 --- a/sunrpc/pmap_rmt.c +++ b/sunrpc/pmap_rmt.c @@ -42,6 +42,7 @@ #include #include #include +#include #include #include #undef _POSIX_SOURCE /* Ultrix needs --roland@gnu */ @@ -238,7 +239,7 @@ clnt_broadcast (prog, vers, proc, xargs, argsp, xresults, resultsp, eachresult) * initialization: create a socket, a broadcast address, and * preserialize the arguments into a send buffer. */ - if ((sock = __socket (AF_INET, SOCK_DGRAM, IPPROTO_UDP)) < 0) + if ((sock = __socket_cloexec (AF_INET, SOCK_DGRAM, IPPROTO_UDP, true)) < 0) { perror (_("Cannot create socket for broadcast rpc")); stat = RPC_CANTSEND; diff --git a/sunrpc/rtime.c b/sunrpc/rtime.c index 79d55d1..48f4681 100644 --- a/sunrpc/rtime.c +++ b/sunrpc/rtime.c @@ -45,6 +45,7 @@ #include #include #include +#include #include #include #include @@ -84,7 +85,7 @@ rtime (struct sockaddr_in *addrp, struct rpc_timeval *timep, else type = SOCK_DGRAM; - s = __socket (AF_INET, type, 0); + s = __socket_cloexec (AF_INET, type, 0, true); if (s < 0) return (-1); diff --git a/sunrpc/svc_tcp.c b/sunrpc/svc_tcp.c index 92886f0..eaf6297 100644 --- a/sunrpc/svc_tcp.c +++ b/sunrpc/svc_tcp.c @@ -58,6 +58,7 @@ #include #include #include +#include #include #include #include @@ -159,7 +160,8 @@ svctcp_create (int sock, u_int sendsize, u_int recvsize) if (sock == RPC_ANYSOCK) { - if ((sock = __socket (AF_INET, SOCK_STREAM, IPPROTO_TCP)) < 0) + if ((sock = __socket_cloexec (AF_INET, SOCK_STREAM, IPPROTO_TCP, + true)) < 0) { perror (_("svc_tcp.c - tcp socket creation problem")); return (SVCXPRT *) NULL; diff --git a/sunrpc/svc_udp.c b/sunrpc/svc_udp.c index 411234a..f7a6da1 100644 --- a/sunrpc/svc_udp.c +++ b/sunrpc/svc_udp.c @@ -55,6 +55,7 @@ #include #include #include +#include #include #include @@ -132,7 +133,8 @@ svcudp_bufcreate (sock, sendsz, recvsz) if (sock == RPC_ANYSOCK) { - if ((sock = __socket (AF_INET, SOCK_DGRAM, IPPROTO_UDP)) < 0) + if ((sock = __socket_cloexec (AF_INET, SOCK_DGRAM, IPPROTO_UDP, + true)) < 0) { perror (_("svcudp_create: socket creation problem")); return (SVCXPRT *) NULL; diff --git a/sunrpc/svc_unix.c b/sunrpc/svc_unix.c index 6d93c5f..0d7466d 100644 --- a/sunrpc/svc_unix.c +++ b/sunrpc/svc_unix.c @@ -58,6 +58,7 @@ #include #include #include +#include #include #include #include @@ -157,7 +158,7 @@ svcunix_create (int sock, u_int sendsize, u_int recvsize, char *path) if (sock == RPC_ANYSOCK) { - if ((sock = __socket (AF_UNIX, SOCK_STREAM, 0)) < 0) + if ((sock = __socket_cloexec (AF_UNIX, SOCK_STREAM, 0, true)) < 0) { perror (_("svc_unix.c - AF_UNIX socket creation problem")); return (SVCXPRT *) NULL; diff --git a/sysdeps/gnu/ifaddrs.c b/sysdeps/gnu/ifaddrs.c index 1b8775f..70db461 100644 --- a/sysdeps/gnu/ifaddrs.c +++ b/sysdeps/gnu/ifaddrs.c @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -37,7 +38,7 @@ getifaddrs (struct ifaddrs **ifap) /* This implementation handles only IPv4 interfaces. The various ioctls below will only work on an AF_INET socket. Some different mechanism entirely must be used for IPv6. */ - int fd = __socket (AF_INET, SOCK_DGRAM, 0); + int fd = __socket_cloexec (AF_INET, SOCK_DGRAM, 0, true); struct ifreq *ifreqs; int nifs; diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c index 8f392b9..3cbd033 100644 --- a/sysdeps/posix/getaddrinfo.c +++ b/sysdeps/posix/getaddrinfo.c @@ -52,6 +52,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #include #include #include +#include #include #include #include @@ -2512,7 +2513,7 @@ getaddrinfo (const char *name, const char *service, close_retry: close_not_cancel_no_status (fd); af = q->ai_family; - fd = __socket (af, SOCK_DGRAM, IPPROTO_IP); + fd = __socket_cloexec (af, SOCK_DGRAM, IPPROTO_IP, true); } else { diff --git a/sysdeps/unix/sysv/linux/check_native.c b/sysdeps/unix/sysv/linux/check_native.c index 68adeb8..4a2e143 100644 --- a/sysdeps/unix/sysv/linux/check_native.c +++ b/sysdeps/unix/sysv/linux/check_native.c @@ -28,6 +28,8 @@ #include #include #include +#include +#include #include #include @@ -40,7 +42,7 @@ void __check_native (uint32_t a1_index, int *a1_native, uint32_t a2_index, int *a2_native) { - int fd = __socket (PF_NETLINK, SOCK_RAW, NETLINK_ROUTE); + int fd = __socket_cloexec (PF_NETLINK, SOCK_RAW, NETLINK_ROUTE, true); struct sockaddr_nl nladdr; memset (&nladdr, '\0', sizeof (nladdr)); diff --git a/sysdeps/unix/sysv/linux/check_pf.c b/sysdeps/unix/sysv/linux/check_pf.c index 976f249e..a098068 100644 --- a/sysdeps/unix/sysv/linux/check_pf.c +++ b/sysdeps/unix/sysv/linux/check_pf.c @@ -26,6 +26,7 @@ #include #include #include +#include #include #include @@ -353,7 +354,7 @@ __check_pf (bool *seen_ipv4, bool *seen_ipv6, } else { - int fd = __socket (PF_NETLINK, SOCK_RAW, NETLINK_ROUTE); + int fd = __socket_cloexec (PF_NETLINK, SOCK_RAW, NETLINK_ROUTE, true); if (__glibc_likely (fd >= 0)) { diff --git a/sysdeps/unix/sysv/linux/ifaddrs.c b/sysdeps/unix/sysv/linux/ifaddrs.c index a47b2ed..aed494e 100644 --- a/sysdeps/unix/sysv/linux/ifaddrs.c +++ b/sysdeps/unix/sysv/linux/ifaddrs.c @@ -29,6 +29,7 @@ #include #include #include +#include #include #include #include @@ -251,7 +252,7 @@ __netlink_open (struct netlink_handle *h) { struct sockaddr_nl nladdr; - h->fd = __socket (PF_NETLINK, SOCK_RAW, NETLINK_ROUTE); + h->fd = __socket_cloexec (PF_NETLINK, SOCK_RAW, NETLINK_ROUTE, true); if (h->fd < 0) goto out;