From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/5177 Path: news.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [UGLY PATCH v3] Support for no-legacy-syscalls archs Date: Thu, 29 May 2014 17:46:21 -0400 Message-ID: <20140529214621.GQ507@brightrain.aerifal.cx> References: <20140525054237.GA18085@brightrain.aerifal.cx> <20140527052625.GC507@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: ger.gmane.org 1401400004 4441 80.91.229.3 (29 May 2014 21:46:44 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Thu, 29 May 2014 21:46:44 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-5182-gllmg-musl=m.gmane.org@lists.openwall.com Thu May 29 23:46:37 2014 Return-path: Envelope-to: gllmg-musl@plane.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1Wq89l-0003E1-1j for gllmg-musl@plane.gmane.org; Thu, 29 May 2014 23:46:37 +0200 Original-Received: (qmail 26322 invoked by uid 550); 29 May 2014 21:46:34 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Original-Received: (qmail 26314 invoked from network); 29 May 2014 21:46:34 -0000 Content-Disposition: inline In-Reply-To: <20140527052625.GC507@brightrain.aerifal.cx> User-Agent: Mutt/1.5.21 (2010-09-15) Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:5177 Archived-At: On Tue, May 27, 2014 at 01:26:25AM -0400, Rich Felker wrote: > diff --git a/src/exit/exit.c b/src/exit/exit.c > index 353f50b..6ac90c3 100644 > --- a/src/exit/exit.c > +++ b/src/exit/exit.c > @@ -1,5 +1,6 @@ > #include > #include > +#include "futex.h" > #include "libc.h" > #include "atomic.h" > #include "syscall.h" > @@ -24,7 +25,12 @@ _Noreturn void exit(int code) > static int lock; > > /* If more than one thread calls exit, hang until _Exit ends it all */ > - while (a_swap(&lock, 1)) __syscall(SYS_pause); > + while (a_swap(&lock, 1)) > +#ifdef SYS_pausex > + __syscall(SYS_pause); > +#else > + __syscall(SYS_futex, &lock, FUTEX_WAIT, 1, 0); > +#endif Removed this change simply by dropping the code (see commit 2e55da911896a91e95b24ab5dc8a9d9b0718f4de). > diff --git a/src/process/posix_spawn.c b/src/process/posix_spawn.c > index 08644f5..6fa33cb 100644 > --- a/src/process/posix_spawn.c > +++ b/src/process/posix_spawn.c > @@ -22,6 +22,18 @@ struct args { > > void __get_handler_set(sigset_t *); > > +static int do_dup2(int old, int new) > +{ > + int r; > +#ifdef SYS_dup2 > + while ((r=__syscall(SYS_dup2, old, new))==-EBUSY); > +#else > + while ((r=__syscall(SYS_dup3, old, new, 0))==-EBUSY); > + if (r==-EINVAL && old==new) return new; > +#endif > + return r; > +} > + This is overkill; I mistakenly thought the use of dup2 in posix_spawn was missing the EBUSY check and loop and added all this mess. But the EBUSY condition cannot happen in a single-threaded process that does not share (CLONE_FILES) its descriptor table. In fact, it seems like the EBUSY condition might be something we can drop completely from dup2.c and dup3.c too. I'm not 100% sure yet but it looks like it can only happen when the application invokes UB by misusing dup2/dup3 in a multithreaded process when the destination descriptor is not already known by the caller to be occupied. > diff --git a/src/select/select.c b/src/select/select.c > index f93597b..7b5f6dc 100644 > --- a/src/select/select.c > +++ b/src/select/select.c > @@ -1,8 +1,26 @@ > #include > +#include > +#include > +#include > #include "syscall.h" > #include "libc.h" > > int select(int n, fd_set *restrict rfds, fd_set *restrict wfds, fd_set *restrict efds, struct timeval *restrict tv) > { > +#ifdef SYS_select > return syscall_cp(SYS_select, n, rfds, wfds, efds, tv); > +#else > + syscall_arg_t data[2] = { 0, _NSIG/8 }; > + struct timespec ts; > + if (tv) { > + if (tv->tv_sec < 0 || tv->tv_usec < 0) > + return __syscall_ret(-EINVAL); > + time_t extra_secs = tv->tv_usec / 1000000; > + ts.tv_nsec = tv->tv_usec % 1000000 * 1000; > + const time_t max_time = (1ULL<<8*sizeof(time_t)-1)-1; > + ts.tv_sec = extra_secs > max_time - tv->tv_sec ? > + max_time : tv->tv_sec + extra_secs; > + } > + return syscall_cp(SYS_pselect6, n, rfds, wfds, efds, tv ? &ts : 0, data); > +#endif This is a lot uglier than I'd prefer, but I don't see any way to improve it. Blame the kernel folks... > } > diff --git a/src/stat/chmod.c b/src/stat/chmod.c > index beb66e5..d4f53c5 100644 > --- a/src/stat/chmod.c > +++ b/src/stat/chmod.c > @@ -1,7 +1,12 @@ > #include > +#include > #include "syscall.h" > > int chmod(const char *path, mode_t mode) > { > +#ifdef SYS_chmod > return syscall(SYS_chmod, path, mode); > +#else > + return syscall(SYS_fchmodat, AT_FDCWD, path, mode); > +#endif > } > diff --git a/src/stat/fchmod.c b/src/stat/fchmod.c > index 6d28141..93e1b64 100644 > --- a/src/stat/fchmod.c > +++ b/src/stat/fchmod.c > @@ -13,5 +13,9 @@ int fchmod(int fd, mode_t mode) > > char buf[15+3*sizeof(int)]; > __procfdname(buf, fd); > +#ifdef SYS_chmod > return syscall(SYS_chmod, buf, mode); > +#else > + return syscall(SYS_fchmodat, AT_FDCWD, buf, mode); > +#endif > } > diff --git a/src/stat/fchmodat.c b/src/stat/fchmodat.c > index 12e7ff0..52029ec 100644 > --- a/src/stat/fchmodat.c > +++ b/src/stat/fchmodat.c > @@ -29,7 +29,7 @@ int fchmodat(int fd, const char *path, mode_t mode, int flag) > > __procfdname(proc, fd2); > if (!(ret = __syscall(SYS_stat, proc, &st)) && !S_ISLNK(st.st_mode)) > - ret = __syscall(SYS_chmod, proc, mode); > + ret = __syscall(SYS_fchmodat, AT_FDCWD, proc, mode); > > __syscall(SYS_close, fd2); > return __syscall_ret(ret); > diff --git a/src/stat/futimesat.c b/src/stat/futimesat.c > index dbefc84..5990aa6 100644 > --- a/src/stat/futimesat.c > +++ b/src/stat/futimesat.c > @@ -1,10 +1,23 @@ > #define _GNU_SOURCE > #include > +#include > +#include > #include "syscall.h" > +#include "libc.h" > > -#ifdef SYS_futimesat > -int futimesat(int dirfd, const char *pathname, const struct timeval times[2]) > +int __futimesat(int dirfd, const char *pathname, const struct timeval times[2]) > { > - return syscall(SYS_futimesat, dirfd, pathname, times); > + struct timespec ts[2]; > + if (times) { > + int i; > + for (i=0; i<2; i++) { > + if (times[i].tv_usec >= 1000000U) I think there's a bug here; whether comparison against 1000000U will promote the LHS to unsigned depends on the definition of suseconds_t. > diff --git a/src/stat/utimensat.c b/src/stat/utimensat.c > index 929698b..abc8d74 100644 > --- a/src/stat/utimensat.c > +++ b/src/stat/utimensat.c > @@ -1,7 +1,40 @@ > #include > +#include > +#include > +#include > #include "syscall.h" > > int utimensat(int fd, const char *path, const struct timespec times[2], int flags) > { > - return syscall(SYS_utimensat, fd, path, times, flags); > + int r = __syscall(SYS_utimensat, fd, path, times, flags); > + if (r != -ENOSYS || flags) return __syscall_ret(r); > + Not a big deal, but this check can be moved inside the #ifdef: > +#ifdef SYS_futimesat > [...] > > diff --git a/src/unistd/dup2.c b/src/unistd/dup2.c > index 87a0d44..ed04a89 100644 > --- a/src/unistd/dup2.c > +++ b/src/unistd/dup2.c > @@ -5,6 +5,11 @@ > int dup2(int old, int new) > { > int r; > +#ifdef SYS_dup2 > while ((r=__syscall(SYS_dup2, old, new))==-EBUSY); > +#else > + while ((r=__syscall(SYS_dup3, old, new, 0))==-EBUSY); > + if (r==-EINVAL && old==new) return new; > +#endif Sadly this implementation is not correct since the kernel checks for EINVAL before EBADF; it causes dup2(fd,fd) to always report success, even if fd is not a valid file descriptor. So I think we really need an ugly check for validity first... Rich