* Re: [PATCH] Be more precise using va_arg in fcntl
2017-06-19 18:10 [PATCH] Be more precise using va_arg in fcntl Omer Anson
@ 2017-06-21 1:08 ` Rich Felker
2017-06-21 17:22 ` Omer Anson
0 siblings, 1 reply; 3+ messages in thread
From: Rich Felker @ 2017-06-21 1:08 UTC (permalink / raw)
To: musl
[-- Attachment #1: Type: text/plain, Size: 5059 bytes --]
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
[-- Attachment #2: fcntl-arg-types.diff --]
[-- Type: text/plain, Size: 1800 bytes --]
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);
+ }
}
[-- Attachment #3: fcntl-type-correct-v2.c --]
[-- Type: text/plain, Size: 1698 bytes --]
#define _GNU_SOURCE
#include <fcntl.h>
#include <stdarg.h>
#include <errno.h>
#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);
}
}
^ permalink raw reply [flat|nested] 3+ messages in thread