[PATCH] Be more precise using va_arg in fcntl

[PATCH] Be more precise using va_arg in fcntl

[Omer Anson]

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
---
 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; \
+		})
+
 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);
+	}
+	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

Re: [PATCH] Be more precise using va_arg in fcntl

[Rich Felker]

[-- 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);
	}
}

[PATCH] Be more precise using va_arg in fcntl

[Omer Anson]

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

This is version 2 of the patch which takes Rich's comments into account:

* The macro was removed. va_arg is called explicitly in every case. I
  wasn't comfortable with the one va_start, many va_end solution, so I
  duplicated the calls to va_start as well (as in the original patch
  posted by Rich).
* Case blocks were removed. Variables were moved up to beginning of
  function.
* In this patch, unknown command handling is unchanged with respect to
  the original code. This is intentional.
---
 src/fcntl/fcntl.c | 78 +++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 61 insertions(+), 17 deletions(-)

diff --git a/src/fcntl/fcntl.c b/src/fcntl/fcntl.c
index ce615d0..1daf4c5 100644
--- a/src/fcntl/fcntl.c
+++ b/src/fcntl/fcntl.c
@@ -7,22 +7,38 @@
 
 int fcntl(int fd, int cmd, ...)
 {
+	int arg_int;
+	struct flock * arg_flock;
+	struct f_owner_ex * arg_owner_ex;
+	struct f_owner_ex ex;
 	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) {
-		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);
+	int ret;
+
+	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:
+		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;
-	}
-	if (cmd == F_DUPFD_CLOEXEC) {
-		int ret = __syscall(SYS_fcntl, fd, F_DUPFD_CLOEXEC, arg);
+	case F_SETFL:
+		va_start(ap, cmd);
+		arg_int = va_arg(ap, int);
+		va_end(ap);
+		arg_int |= O_LARGEFILE;
+		return syscall(SYS_fcntl, fd, cmd, arg_int);
+	case F_DUPFD_CLOEXEC:
+		va_start(ap, cmd);
+		arg_int = va_arg(ap, int);
+		va_end(ap);
+		ret = __syscall(SYS_fcntl, fd, F_DUPFD_CLOEXEC, arg_int);
 		if (ret != -EINVAL) {
 			if (ret >= 0)
 				__syscall(SYS_fcntl, ret, F_SETFD, FD_CLOEXEC);
@@ -33,17 +49,45 @@ int fcntl(int fd, int cmd, ...)
 			if (ret >= 0) __syscall(SYS_close, ret);
 			return __syscall_ret(-EINVAL);
 		}
-		ret = __syscall(SYS_fcntl, fd, F_DUPFD, arg);
+		ret = __syscall(SYS_fcntl, fd, F_DUPFD, arg_int);
 		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:
+		va_start(ap, cmd);
+		arg_int = va_arg(ap, int);
+		va_end(ap);
+		return syscall(SYS_fcntl, fd, cmd, arg_int);
 	case F_GETLK:
+	case F_SETLK:
+	case F_OFD_SETLK:
+	case F_OFD_GETLK:
+		va_start(ap, cmd);
+		arg_flock = va_arg(ap, struct flock *);
+		va_end(ap);
+		return syscall(SYS_fcntl, fd, cmd, arg_flock);
+	case F_OFD_SETLKW:
+	case F_SETLKW:
+		va_start(ap, cmd);
+		arg_flock = va_arg(ap, struct flock *);
+		va_end(ap);
+		return syscall_cp(SYS_fcntl, fd, cmd, arg_flock);
 	case F_GETOWN_EX:
 	case F_SETOWN_EX:
-		return syscall(SYS_fcntl, fd, cmd, (void *)arg);
+		va_start(ap, cmd);
+		arg_owner_ex = va_arg(ap, struct f_owner_ex *);
+		va_end(ap);
+		return syscall(SYS_fcntl, fd, cmd, arg_owner_ex);
 	default:
+		va_start(ap, cmd);
+		arg = va_arg(ap, unsigned long);
+		va_end(ap);
 		return syscall(SYS_fcntl, fd, cmd, arg);
 	}
 }
-- 
2.4.11