use of varargs in open and various functions

[Norbert Lange <nolange79@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2177 bytes --]

Hello,

I had some dealings with software that forwards arguments from vararg functions,
the kind with optional but known fixed type.
open, fcntl, ioctl are some examples.

What happens in musl is that the optional argument is optionally or
always retrieved (fcntl).
I think this is pretty much pointless, the only reason to use varags would be:

1.   varying argument types (plainly not possibly to replace with
direct arguments)
2.   different parameter passing than direct arguments (ABI compatibility)
3.   accessing parameters the caller has not provided.

1. disqualifies functions like printf which I am not planning to touch.
2. would need more tests, but I expect this to be true for all modern
architectures,
    atleast its true for most of them.
3. thats a thing for open where the 3rd argument is only accessed if
certain flags are set,
    but most other functions in musl seem to always access "optional" arguments.
    reading theses non-existing arguments could trigger tools like
valgrind, but other
    than that it should not be an issue aslong a couple bytes of stack
is available.

I tested code calling the functions, I believe calls are identical for
varags and normal functions,
for all architectures that are supported by musl. [1]
(sidenote: clang generates absolutely awful code for those varag functions).

So, is there any reason not to make this the default for musl (adding
a fallback option via a define)? I am running a few tools (bash, nano)
compiled with the applied patch and so far I have no adverse effects,
it should not affect ABI for machines satisfying 2. and it gets rid of
some wacky code that in the end just passes in another register (like
open).

Further I would like to add that Torvalds shares a similar view [2].

Kind regards,
Norbert

[1] https://godbolt.org/z/asBT92
[2] https://lkml.org/lkml/2014/10/30/812

PS. why is this a thing in open:
  int fd = __sys_open_cp(filename, flags, mode);
  if (fd>=0 && (flags & O_CLOEXEC))
  __syscall(SYS_fcntl, fd, F_SETFD, FD_CLOEXEC);

Is this to support old kernels?
I thought O_CLOEXEC is used to fight races during a fork,
so if its not supported by the kernel it wont do alot.

[-- Attachment #2: lessvarargs.patch --]
[-- Type: text/x-patch, Size: 5933 bytes --]

diff -burN musl-1.1.21.org/src/fcntl/fcntl.c musl-1.1.21/src/fcntl/fcntl.c
--- musl-1.1.21.org/src/fcntl/fcntl.c	2019-04-10 13:35:43.589279396 +0200
+++ musl-1.1.21/src/fcntl/fcntl.c	2019-04-10 13:53:46.979931879 +0200
@@ -1,16 +1,12 @@
 #define _GNU_SOURCE
+#define fcntl undef_fcntl
 #include <fcntl.h>
-#include <stdarg.h>
 #include <errno.h>
 #include "syscall.h"
+#undef fcntl
 
-int fcntl(int fd, int cmd, ...)
+int fcntl(int fd, int cmd, unsigned long arg)
 {
-	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) {
diff -burN musl-1.1.21.org/src/fcntl/openat.c musl-1.1.21/src/fcntl/openat.c
--- musl-1.1.21.org/src/fcntl/openat.c	2019-04-10 13:35:43.589279396 +0200
+++ musl-1.1.21/src/fcntl/openat.c	2019-04-10 13:53:58.088037210 +0200
@@ -1,18 +1,10 @@
+#define openat undef_openat
 #include <fcntl.h>
-#include <stdarg.h>
 #include "syscall.h"
+#undef openat
 
-int openat(int fd, const char *filename, int flags, ...)
+int openat(int fd, const char *filename, int flags, mode_t mode)
 {
-	mode_t mode = 0;
-
-	if ((flags & O_CREAT) || (flags & O_TMPFILE) == O_TMPFILE) {
-		va_list ap;
-		va_start(ap, flags);
-		mode = va_arg(ap, mode_t);
-		va_end(ap);
-	}
-
 	return syscall_cp(SYS_openat, fd, filename, flags|O_LARGEFILE, mode);
 }
 
diff -burN musl-1.1.21.org/src/fcntl/open.c musl-1.1.21/src/fcntl/open.c
--- musl-1.1.21.org/src/fcntl/open.c	2019-04-10 13:35:43.589279396 +0200
+++ musl-1.1.21/src/fcntl/open.c	2019-04-10 13:53:53.939997879 +0200
@@ -1,18 +1,10 @@
+#define open undef_open
 #include <fcntl.h>
-#include <stdarg.h>
 #include "syscall.h"
+#undef open
 
-int open(const char *filename, int flags, ...)
+int open(const char *filename, int flags, mode_t mode)
 {
-	mode_t mode = 0;
-
-	if ((flags & O_CREAT) || (flags & O_TMPFILE) == O_TMPFILE) {
-		va_list ap;
-		va_start(ap, flags);
-		mode = va_arg(ap, mode_t);
-		va_end(ap);
-	}
-
 	int fd = __sys_open_cp(filename, flags, mode);
 	if (fd>=0 && (flags & O_CLOEXEC))
 		__syscall(SYS_fcntl, fd, F_SETFD, FD_CLOEXEC);
diff -burN musl-1.1.21.org/src/legacy/ulimit.c musl-1.1.21/src/legacy/ulimit.c
--- musl-1.1.21.org/src/legacy/ulimit.c	2019-04-10 13:35:43.589279396 +0200
+++ musl-1.1.21/src/legacy/ulimit.c	2019-04-10 14:03:23.113362412 +0200
@@ -1,17 +1,13 @@
+#define ulimit undef_ulimit
 #include <sys/resource.h>
 #include <ulimit.h>
-#include <stdarg.h>
+#undef ulimit
 
-long ulimit(int cmd, ...)
+long ulimit(int cmd, long val)
 {
 	struct rlimit rl;
 	getrlimit(RLIMIT_FSIZE, &rl);
 	if (cmd == UL_SETFSIZE) {
-		long val;
-		va_list ap;
-		va_start(ap, cmd);
-		val = va_arg(ap, long);
-		va_end(ap);
 		rl.rlim_cur = 512ULL * val;
 		if (setrlimit(RLIMIT_FSIZE, &rl)) return -1;
 	}
diff -burN musl-1.1.21.org/src/linux/clone.c musl-1.1.21/src/linux/clone.c
--- musl-1.1.21.org/src/linux/clone.c	2019-04-10 13:35:43.589279396 +0200
+++ musl-1.1.21/src/linux/clone.c	2019-04-10 14:00:41.015839619 +0200
@@ -1,21 +1,13 @@
 #define _GNU_SOURCE
-#include <stdarg.h>
+#define clone undef_clone
 #include <unistd.h>
 #include <sched.h>
 #include "pthread_impl.h"
 #include "syscall.h"
+#undef clone
 
-int clone(int (*func)(void *), void *stack, int flags, void *arg, ...)
+int clone(int (*func)(void *), void *stack, int flags, void *arg,
+	pid_t *ptid, void *tls, pid_t *ctid)
 {
-	va_list ap;
-	pid_t *ptid, *ctid;
-	void  *tls;
-
-	va_start(ap, arg);
-	ptid = va_arg(ap, pid_t *);
-	tls  = va_arg(ap, void *);
-	ctid = va_arg(ap, pid_t *);
-	va_end(ap);
-
 	return __syscall_ret(__clone(func, stack, flags, arg, ptid, tls, ctid));
 }
diff -burN musl-1.1.21.org/src/linux/prctl.c musl-1.1.21/src/linux/prctl.c
--- musl-1.1.21.org/src/linux/prctl.c	2019-04-10 13:35:43.589279396 +0200
+++ musl-1.1.21/src/linux/prctl.c	2019-04-10 13:57:26.570008404 +0200
@@ -1,14 +1,9 @@
+#define prctl undef_prctl
 #include <sys/prctl.h>
-#include <stdarg.h>
 #include "syscall.h"
+#undef prctl
 
-int prctl(int op, ...)
+int prctl(int op, unsigned long x0, unsigned long x1, unsigned long x2, unsigned long x3)
 {
-	unsigned long x[4];
-	int i;
-	va_list ap;
-	va_start(ap, op);
-	for (i=0; i<4; i++) x[i] = va_arg(ap, unsigned long);
-	va_end(ap);
-	return syscall(SYS_prctl, op, x[0], x[1], x[2], x[3]);
+	return syscall(SYS_prctl, op, x0, x1, x2, x3);
 }
diff -burN musl-1.1.21.org/src/linux/ptrace.c musl-1.1.21/src/linux/ptrace.c
--- musl-1.1.21.org/src/linux/ptrace.c	2019-04-10 13:35:43.589279396 +0200
+++ musl-1.1.21/src/linux/ptrace.c	2019-04-10 14:01:09.280105349 +0200
@@ -1,26 +1,13 @@
+#define ptrace undef_ptrace
 #include <sys/ptrace.h>
-#include <stdarg.h>
 #include <unistd.h>
 #include "syscall.h"
+#undef ptrace
 
-long ptrace(int req, ...)
+long ptrace(int req, pid_t pid, void *addr, void *data, void *addr2)
 {
-	va_list ap;
-	pid_t pid;
-	void *addr, *data, *addr2 = 0;
 	long ret, result;
 
-	va_start(ap, req);
-	pid = va_arg(ap, pid_t);
-	addr = va_arg(ap, void *);
-	data = va_arg(ap, void *);
-	/* PTRACE_{READ,WRITE}{DATA,TEXT} (16...19) are specific to SPARC. */
-#ifdef PTRACE_READDATA
-	if ((unsigned)req - PTRACE_READDATA < 4)
-		addr2 = va_arg(ap, void *);
-#endif
-	va_end(ap);
-
 	if (req-1U < 3) data = &result;
 	ret = syscall(SYS_ptrace, req, pid, addr, data, addr2);
 
diff -burN musl-1.1.21.org/src/misc/ioctl.c musl-1.1.21/src/misc/ioctl.c
--- musl-1.1.21.org/src/misc/ioctl.c	2019-04-10 13:35:43.605279563 +0200
+++ musl-1.1.21/src/misc/ioctl.c	2019-04-10 14:01:52.920515470 +0200
@@ -1,13 +1,9 @@
+#define ioctl undef_ioctl
 #include <sys/ioctl.h>
-#include <stdarg.h>
 #include "syscall.h"
+#undef ioctl
 
-int ioctl(int fd, int req, ...)
+int ioctl(int fd, int req, void *arg)
 {
-	void *arg;
-	va_list ap;
-	va_start(ap, req);
-	arg = va_arg(ap, void *);
-	va_end(ap);
 	return syscall(SYS_ioctl, fd, req, arg);
 }