[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 29 May 2014 17:46:21 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [UGLY PATCH v3] Support for no-legacy-syscalls archs
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 <stdlib.h>
> #include <stdint.h>
> +#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 <sys/select.h>
> +#include <signal.h>
> +#include <stdint.h>
> +#include <errno.h>
> #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 <sys/stat.h>
> +#include <fcntl.h>
> #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 <sys/time.h>
> +#include <sys/stat.h>
> +#include <errno.h>
> #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 <sys/stat.h>
> +#include <sys/time.h>
> +#include <fcntl.h>
> +#include <errno.h>
> #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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
