From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/11543 Path: news.gmane.org!.POSTED!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] Be more precise using va_arg in fcntl Date: Tue, 20 Jun 2017 21:08:11 -0400 Message-ID: <20170621010811.GC1627@brightrain.aerifal.cx> References: <1497895843-4631-1-git-send-email-oaanson@gmail.com> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="llIrKcgUOe3dCx0c" X-Trace: blaine.gmane.org 1498007305 9629 195.159.176.226 (21 Jun 2017 01:08:25 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Wed, 21 Jun 2017 01:08:25 +0000 (UTC) User-Agent: Mutt/1.5.21 (2010-09-15) To: musl@lists.openwall.com Original-X-From: musl-return-11556-gllmg-musl=m.gmane.org@lists.openwall.com Wed Jun 21 03:08:21 2017 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.84_2) (envelope-from ) id 1dNU85-0002K2-3x for gllmg-musl@m.gmane.org; Wed, 21 Jun 2017 03:08:21 +0200 Original-Received: (qmail 1842 invoked by uid 550); 21 Jun 2017 01:08:24 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 1821 invoked from network); 21 Jun 2017 01:08:23 -0000 Content-Disposition: inline In-Reply-To: <1497895843-4631-1-git-send-email-oaanson@gmail.com> Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:11543 Archived-At: --llIrKcgUOe3dCx0c Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Mon, Jun 19, 2017 at 09:10:43PM +0300, Omer Anson wrote: > Currently, fcntl automatically calls va_arg with type unsigned long, > regardless of the actual invoked command, and therefore ignoring whether > such an argument exists, or its type. > > According to the C standard[1][2], calling va_arg if the types do not match, > or beyond the end of the parameter list, causes an undefined behaviour. > > This change modifies fcntl to try and call va_arg only when necessary, > and with the correct parameter type. It relies on the cmd argument to > know what's the expected command. In case the cmd is unknown, it falls > back to the previous behaviour. > > [1] http://www.open-std.org/jtc1/sc22/wg14/www/standards > [2] http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/stdarg.h.html Thanks for bringing this up again. I've actually worked on this at least twice in the past, and the work got set aside because of bikeshedding and/or theoretical concerns about how to handle unknown commands or something -- but it appears all the discussion was on IRC; I can't find it on the list. I do have some of my old versions of the patch, but I'd need to cross-reference them against commits with later dates because some related changes (making pointers get passed right to kernel on x32, I think) that overlap with my patches were already committed. Some comments: > --- > src/fcntl/fcntl.c | 72 ++++++++++++++++++++++++++++++++++++++++++------------- > 1 file changed, 56 insertions(+), 16 deletions(-) > > diff --git a/src/fcntl/fcntl.c b/src/fcntl/fcntl.c > index ce615d0..9d6b2cc 100644 > --- a/src/fcntl/fcntl.c > +++ b/src/fcntl/fcntl.c > @@ -5,24 +5,40 @@ > #include "syscall.h" > #include "libc.h" > > +#define GET_ARG_MACRO(type) ({ \ > + va_list ap; \ > + va_start(ap, cmd); \ > + type __arg = va_arg(ap, type); \ > + va_end(ap); \ > + __arg; \ > + }) > + This macro uses GNU C statement-expressions, which aren't allowed in musl, but it also seems mostly gratuitous. The first 2 lines can just be at the top of the function as before; only the va_end then ends up being repeate when expanding it manually. > int fcntl(int fd, int cmd, ...) > { > - unsigned long arg; > - va_list ap; > - va_start(ap, cmd); > - arg = va_arg(ap, unsigned long); > - va_end(ap); > - if (cmd == F_SETFL) arg |= O_LARGEFILE; > - if (cmd == F_SETLKW) return syscall_cp(SYS_fcntl, fd, cmd, (void *)arg); > - if (cmd == F_GETOWN) { > + switch (cmd) { > + case F_GETFD: > + case F_GETFL: > + case F_GETSIG: > + case F_GETLEASE: > + case F_GET_SEALS: > + case F_GETPIPE_SZ: > + return syscall(SYS_fcntl, fd, cmd); > + case F_GETOWN: { > struct f_owner_ex ex; > int ret = __syscall(SYS_fcntl, fd, F_GETOWN_EX, &ex); > - if (ret == -EINVAL) return __syscall(SYS_fcntl, fd, cmd, (void *)arg); > + if (ret == -EINVAL) return __syscall(SYS_fcntl, fd, cmd); > if (ret) return __syscall_ret(ret); > return ex.type == F_OWNER_PGRP ? -ex.pid : ex.pid; > } > - if (cmd == F_DUPFD_CLOEXEC) { > - int ret = __syscall(SYS_fcntl, fd, F_DUPFD_CLOEXEC, arg); > + case F_SETFL: { > + int arg = GET_ARG_MACRO(int); > + arg |= O_LARGEFILE; > + return syscall(SYS_fcntl, fd, cmd, arg); > + } The brace placement for case blocks like this isn't really idiomatic in musl. It's not horrible either, but might be nicer to declare the variables at the top. > + case F_DUPFD_CLOEXEC: { > + int ret; > + int arg = GET_ARG_MACRO(int); > + ret = __syscall(SYS_fcntl, fd, F_DUPFD_CLOEXEC, arg); > if (ret != -EINVAL) { > if (ret >= 0) > __syscall(SYS_fcntl, ret, F_SETFD, FD_CLOEXEC); > @@ -37,13 +53,37 @@ int fcntl(int fd, int cmd, ...) > if (ret >= 0) __syscall(SYS_fcntl, ret, F_SETFD, FD_CLOEXEC); > return __syscall_ret(ret); > } > - switch (cmd) { > - case F_SETLK: > + case F_DUPFD: > + case F_SETFD: > + case F_SETSIG: > + case F_SETLEASE: > + case F_ADD_SEALS: > + case F_SETOWN: > + case F_SETPIPE_SZ: > + case F_NOTIFY: { > + int arg = GET_ARG_MACRO(int); > + return syscall(SYS_fcntl, fd, cmd, arg); > + } > case F_GETLK: > + case F_SETLK: > + case F_OFD_SETLK: > + case F_OFD_GETLK: { > + struct flock * arg = GET_ARG_MACRO(struct flock *); > + return syscall(SYS_fcntl, fd, cmd, arg); > + } > + case F_OFD_SETLKW: > + case F_SETLKW: { > + struct flock * arg = GET_ARG_MACRO(struct flock *); > + return syscall_cp(SYS_fcntl, fd, cmd, arg); > + } > case F_GETOWN_EX: > - case F_SETOWN_EX: > - return syscall(SYS_fcntl, fd, cmd, (void *)arg); > - default: > + case F_SETOWN_EX: { > + struct f_owner_ex * arg = GET_ARG_MACRO(struct f_owner_ex *); > return syscall(SYS_fcntl, fd, cmd, arg); > } > + default: { > + unsigned long arg = GET_ARG_MACRO(unsigned long); > + return syscall(SYS_fcntl, fd, cmd, arg); > + } > + } > } > -- > 2.4.11 I'm attaching my old versions of the patch for reference, in case they suggest nicer ways to do anything. One is a patch that was already partially applied, I think (see above); the other is just a replacement .c file since the patch is not very readable. Rich --llIrKcgUOe3dCx0c Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="fcntl-arg-types.diff" diff --git a/src/fcntl/fcntl.c b/src/fcntl/fcntl.c index 4c34ba0..7b30d2a 100644 --- a/src/fcntl/fcntl.c +++ b/src/fcntl/fcntl.c @@ -9,15 +9,48 @@ int fcntl(int fd, int cmd, ...) { long arg; va_list ap; + va_start(ap, cmd); - arg = va_arg(ap, long); + switch (cmd) { + case F_DUPFD: + case F_DUPFD_CLOEXEC: + case F_SETFD: + case F_SETFL: + case F_SETOWN: + case F_SETSIG: + case F_SETLEASE: + case F_NOTIFY: + case F_SETPIPE_SZ: + arg = va_arg(ap, int); + break; + case F_GETFD: + case F_GETFL: + case F_GETOWN: + case F_GETSIG: + case F_GETLEASE: + case F_GETPIPE_SZ: + arg = 0; + break; + case F_SETLK: + case F_SETLKW: + case F_GETLK: + arg = (long)va_arg(ap, struct flock *); + break; + case F_GETOWN_EX: + case F_SETOWN_EX: + arg = (long)va_arg(ap, struct f_owner_ex *); + break; + default: + arg = va_arg(ap, long); + } va_end(ap); + if (cmd == F_SETFL) arg |= O_LARGEFILE; - if (cmd == F_SETLKW) return syscall_cp(SYS_fcntl, fd, cmd, arg); + if (cmd == F_SETLKW) return syscall_cp(SYS_fcntl, fd, cmd, (void *)arg); if (cmd == F_GETOWN) { struct f_owner_ex ex; int ret = __syscall(SYS_fcntl, fd, F_GETOWN_EX, &ex); - if (ret == -EINVAL) return __syscall(SYS_fcntl, fd, cmd, arg); + if (ret == -EINVAL) return __syscall(SYS_fcntl, fd, cmd, (void *)arg); if (ret) return __syscall_ret(ret); return ex.type == F_OWNER_PGRP ? -ex.pid : ex.pid; } @@ -37,5 +70,14 @@ int fcntl(int fd, int cmd, ...) if (ret >= 0) __syscall(SYS_fcntl, ret, F_SETFD, FD_CLOEXEC); return __syscall_ret(ret); } - return syscall(SYS_fcntl, fd, cmd, arg); + switch (cmd) { + case F_SETLK: + case F_SETLKW: + case F_GETLK: + case F_GETOWN_EX: + case F_SETOWN_EX: + return syscall(SYS_fcntl, fd, cmd, (void *)arg); + default: + return syscall(SYS_fcntl, fd, cmd, arg); + } } --llIrKcgUOe3dCx0c Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="fcntl-type-correct-v2.c" #define _GNU_SOURCE #include #include #include #include "syscall.h" #include "libc.h" int fcntl(int fd, int cmd, ...) { va_list ap; int arg, ret; void *ptrarg; struct f_owner_ex ex; switch (cmd) { case F_GETFD: case F_GETFL: case F_GETSIG: case F_GETLEASE: case F_GETPIPE_SZ: return syscall(SYS_fcntl, fd, cmd); case F_GETOWN: ret = __syscall(SYS_fcntl, fd, F_GETOWN_EX, &ex); if (ret == -EINVAL) return __syscall(SYS_fcntl, fd, cmd); if (ret) return __syscall_ret(ret); return ex.type == F_OWNER_PGRP ? -ex.pid : ex.pid; case F_DUPFD: case F_SETFD: case F_SETOWN: case F_SETSIG: va_start(ap, cmd); arg = va_arg(ap, int); va_end(ap); return syscall(SYS_fcntl, fd, cmd, arg); case F_SETFL: va_start(ap, cmd); arg = va_arg(ap, int) | O_LARGEFILE; va_end(ap); return syscall(SYS_fcntl, fd, cmd, arg); case F_DUPFD_CLOEXEC: va_start(ap, cmd); arg = va_arg(ap, int); va_end(ap); ret = __syscall(SYS_fcntl, fd, F_DUPFD_CLOEXEC, arg); if (ret != -EINVAL) { if (ret >= 0) __syscall(SYS_fcntl, ret, F_SETFD, FD_CLOEXEC); return __syscall_ret(ret); } ret = __syscall(SYS_fcntl, fd, F_DUPFD_CLOEXEC, 0); if (ret != -EINVAL) { if (ret >= 0) __syscall(SYS_close, ret); return __syscall_ret(-EINVAL); } ret = __syscall(SYS_fcntl, fd, F_DUPFD, arg); if (ret >= 0) __syscall(SYS_fcntl, ret, F_SETFD, FD_CLOEXEC); return __syscall_ret(ret); case F_SETLKW: va_start(ap, cmd); ptrarg = va_arg(ap, void *); va_end(ap); return syscall_cp(SYS_fcntl, fd, cmd, ptrarg); default: va_start(ap, cmd); ptrarg = va_arg(ap, void *); va_end(ap); return syscall(SYS_fcntl, fd, cmd, ptrarg); } } --llIrKcgUOe3dCx0c--