Message ID | 1379336886-14715-1-git-send-email-edgar.iglesias@gmail.com |
---|---|
State | New |
Headers | show |
Hi, On Mon, Sep 16, 2013 at 03:08:06PM +0200, edgar.iglesias@gmail.com wrote: > From: "Edgar E. Iglesias" <edgar.iglesias@gmail.com> > > If the host lacks support for SOCK_CLOEXEC or SOCK_NONBLOCK, > try to emulate them with fcntl() FD_CLOEXEC and O_NONBLOCK. Last time emulating CLOEXEC with fcntl was discussed, the idea was rejected[1]. The whole point of CLOEXEC flag is guarantee race free open, which when implemented this way it is no longer. It is better to tell the userspace that atomic operation is not available and let the userspace app/library to cope with that, than give the application false sense of safety. Also, this was 4 years ago, on which platforms is this still important? Riku [1] http://lists.nongnu.org/archive/html/qemu-devel/2009-05/msg00263.html > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com> > --- > linux-user/syscall.c | 48 +++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 45 insertions(+), 3 deletions(-) > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index c62d875..6aa8cd7 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -1701,7 +1701,7 @@ static void unlock_iovec(struct iovec *vec, abi_ulong target_addr, > free(vec); > } > > -static inline void target_to_host_sock_type(int *type) > +static inline int target_to_host_sock_type(int *type) > { > int host_type = 0; > int target_type = *type; > @@ -1718,22 +1718,64 @@ static inline void target_to_host_sock_type(int *type) > break; > } > if (target_type & TARGET_SOCK_CLOEXEC) { > +#if defined(SOCK_CLOEXEC) > host_type |= SOCK_CLOEXEC; > +#elif !defined(FD_CLOEXEC) > + return -TARGET_EINVAL; > +#endif > } > if (target_type & TARGET_SOCK_NONBLOCK) { > +#if defined(SOCK_NONBLOCK) > host_type |= SOCK_NONBLOCK; > +#elif !defined(O_NONBLOCK) > + return -TARGET_EINVAL; > +#endif > } > *type = host_type; > + return 0; > +} > + > +/* Try to emulate socket type flags after socket creation. */ > +static int sock_flags_fixup(int fd, int target_type) > +{ > +#if !defined(SOCK_CLOEXEC) && defined(FD_CLOEXEC) > + if (target_type & TARGET_SOCK_CLOEXEC) { > + if (fcntl(fd, F_SETFD, FD_CLOEXEC) == -1) { > + close(fd); > + return -TARGET_EINVAL; > + } > + } > +#endif > +#if !defined(SOCK_NONBLOCK) && defined(O_NONBLOCK) > + if (target_type & TARGET_SOCK_NONBLOCK) { > + int flags = fcntl(fd, F_GETFL); > + if (fcntl(fd, F_SETFL, O_NONBLOCK | flags) == -1) { > + close(fd); > + return -TARGET_EINVAL; > + } > + } > +#endif > + return fd; > } > > /* do_socket() Must return target values and target errnos. */ > static abi_long do_socket(int domain, int type, int protocol) > { > - target_to_host_sock_type(&type); > + int target_type = type; > + int ret; > + > + ret = target_to_host_sock_type(&type); > + if (ret) { > + return ret; > + } > > if (domain == PF_NETLINK) > return -EAFNOSUPPORT; /* do not NETLINK socket connections possible */ > - return get_errno(socket(domain, type, protocol)); > + ret = get_errno(socket(domain, type, protocol)); > + if (ret >= 0) { > + ret = sock_flags_fixup(ret, target_type); > + } > + return ret; > } > > /* do_bind() Must return target values and target errnos. */ > -- > 1.7.10.4
On Mon, Sep 23, 2013 at 12:37:10PM +0300, Riku Voipio wrote: > Hi, Hi Riku, > > On Mon, Sep 16, 2013 at 03:08:06PM +0200, edgar.iglesias@gmail.com wrote: > > From: "Edgar E. Iglesias" <edgar.iglesias@gmail.com> > > > > If the host lacks support for SOCK_CLOEXEC or SOCK_NONBLOCK, > > try to emulate them with fcntl() FD_CLOEXEC and O_NONBLOCK. > > Last time emulating CLOEXEC with fcntl was discussed, the idea > was rejected[1]. The whole point of CLOEXEC flag is guarantee > race free open, which when implemented this way it is no longer. > > It is better to tell the userspace that atomic operation is not > available and let the userspace app/library to cope with that, > than give the application false sense of safety. Agreed, I'll send a v2 that EINVALs on lack of SOCK_CLOEXEC. My primary concern is to avoid the build error of qemu. Thanks, Edgar
diff --git a/linux-user/syscall.c b/linux-user/syscall.c index c62d875..6aa8cd7 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -1701,7 +1701,7 @@ static void unlock_iovec(struct iovec *vec, abi_ulong target_addr, free(vec); } -static inline void target_to_host_sock_type(int *type) +static inline int target_to_host_sock_type(int *type) { int host_type = 0; int target_type = *type; @@ -1718,22 +1718,64 @@ static inline void target_to_host_sock_type(int *type) break; } if (target_type & TARGET_SOCK_CLOEXEC) { +#if defined(SOCK_CLOEXEC) host_type |= SOCK_CLOEXEC; +#elif !defined(FD_CLOEXEC) + return -TARGET_EINVAL; +#endif } if (target_type & TARGET_SOCK_NONBLOCK) { +#if defined(SOCK_NONBLOCK) host_type |= SOCK_NONBLOCK; +#elif !defined(O_NONBLOCK) + return -TARGET_EINVAL; +#endif } *type = host_type; + return 0; +} + +/* Try to emulate socket type flags after socket creation. */ +static int sock_flags_fixup(int fd, int target_type) +{ +#if !defined(SOCK_CLOEXEC) && defined(FD_CLOEXEC) + if (target_type & TARGET_SOCK_CLOEXEC) { + if (fcntl(fd, F_SETFD, FD_CLOEXEC) == -1) { + close(fd); + return -TARGET_EINVAL; + } + } +#endif +#if !defined(SOCK_NONBLOCK) && defined(O_NONBLOCK) + if (target_type & TARGET_SOCK_NONBLOCK) { + int flags = fcntl(fd, F_GETFL); + if (fcntl(fd, F_SETFL, O_NONBLOCK | flags) == -1) { + close(fd); + return -TARGET_EINVAL; + } + } +#endif + return fd; } /* do_socket() Must return target values and target errnos. */ static abi_long do_socket(int domain, int type, int protocol) { - target_to_host_sock_type(&type); + int target_type = type; + int ret; + + ret = target_to_host_sock_type(&type); + if (ret) { + return ret; + } if (domain == PF_NETLINK) return -EAFNOSUPPORT; /* do not NETLINK socket connections possible */ - return get_errno(socket(domain, type, protocol)); + ret = get_errno(socket(domain, type, protocol)); + if (ret >= 0) { + ret = sock_flags_fixup(ret, target_type); + } + return ret; } /* do_bind() Must return target values and target errnos. */