Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Wed, 21 Jun 2017 20:22:39 +0300
From: Omer Anson <oaanson@...il.com>
To: musl@...ts.openwall.com
Cc: Omer Anson <oaanson@...il.com>
Subject: [PATCH] Be more precise using va_arg in fcntl

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

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.