[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 20 Jun 2017 21:08:11 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] Be more precise using va_arg in fcntl
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
View attachment "fcntl-arg-types.diff" of type "text/plain" (1800 bytes)
View attachment "fcntl-type-correct-v2.c" of type "text/plain" (1699 bytes)
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.
