diff mbox series

[v3,2/3] safe_open, safe_openat: Fix undefined behaviour in vararg handling

Message ID 20221130133619.632073-3-tudor.cretu@arm.com
State Superseded
Headers show
Series safe_macros: Fix undefined behaviour in vararg handling | expand

Commit Message

Tudor Cretu Nov. 30, 2022, 1:36 p.m. UTC 
Accessing elements in an empty va_list is undefined behaviour.
The open system call expects the mode argument only when O_CREAT or
O_TMPFILE is specified in flags, otherwise the mode argument is ignored.

Modify the safe_open and safe_openat wrappers to read the variadic
argument only when it's expected to be provided.

Signed-off-by: Tudor Cretu <tudor.cretu@arm.com>
---
 include/lapi/fcntl.h   |  5 +++++
 lib/safe_macros.c      | 21 ++++++++++++---------
 lib/tst_safe_file_at.c | 13 ++++++++-----
 3 files changed, 25 insertions(+), 14 deletions(-)

Comments

Cyril Hrubis Nov. 30, 2022, 2:06 p.m. UTC | #1 
Hi!
> +#ifndef __OPEN_NEEDS_MODE
> + #define __OPEN_NEEDS_MODE(oflag) \
> +	(((oflag) & O_CREAT) != 0 || ((oflag) & O_TMPFILE) == O_TMPFILE)
> +#endif

I suppose that it would be a bit cleaner just to define our macro
TST_OPEN_NEEDS_MODE() instead.

Other than that it looks good.
diff mbox series

Patch

diff --git a/include/lapi/fcntl.h b/include/lapi/fcntl.h
index 8a0a893f9..c99ceb08d 100644
--- a/include/lapi/fcntl.h
+++ b/include/lapi/fcntl.h
@@ -141,6 +141,11 @@ 
 # define MAX_HANDLE_SZ	128
 #endif
 
+#ifndef __OPEN_NEEDS_MODE
+ #define __OPEN_NEEDS_MODE(oflag) \
+	(((oflag) & O_CREAT) != 0 || ((oflag) & O_TMPFILE) == O_TMPFILE)
+#endif
+
 #ifndef HAVE_STRUCT_FILE_HANDLE
 struct file_handle {
 	unsigned int handle_bytes;
diff --git a/lib/safe_macros.c b/lib/safe_macros.c
index d8816631f..af00db960 100644
--- a/lib/safe_macros.c
+++ b/lib/safe_macros.c
@@ -13,7 +13,6 @@ 
 #include <sys/xattr.h>
 #include <sys/sysinfo.h>
 #include <errno.h>
-#include <fcntl.h>
 #include <libgen.h>
 #include <limits.h>
 #include <pwd.h>
@@ -21,6 +20,7 @@ 
 #include <stdlib.h>
 #include <unistd.h>
 #include <malloc.h>
+#include "lapi/fcntl.h"
 #include "test.h"
 #include "safe_macros.h"
 
@@ -236,18 +236,21 @@  int safe_munmap(const char *file, const int lineno, void (*cleanup_fn) (void),
 int safe_open(const char *file, const int lineno, void (*cleanup_fn) (void),
               const char *pathname, int oflags, ...)
 {
-	va_list ap;
 	int rval;
-	mode_t mode;
+	mode_t mode = 0;
 
-	va_start(ap, oflags);
+	if (__OPEN_NEEDS_MODE(oflags)) {
+		va_list ap;
 
-	/* Android's NDK's mode_t is smaller than an int, which results in
-	 * SIGILL here when passing the mode_t type.
-	 */
-	mode = va_arg(ap, int);
+		va_start(ap, oflags);
+
+		/* Android's NDK's mode_t is smaller than an int, which results in
+		 * SIGILL here when passing the mode_t type.
+		 */
+		mode = va_arg(ap, int);
 
-	va_end(ap);
+		va_end(ap);
+	}
 
 	rval = open(pathname, oflags, mode);
 
diff --git a/lib/tst_safe_file_at.c b/lib/tst_safe_file_at.c
index f530dc349..6d0925b74 100644
--- a/lib/tst_safe_file_at.c
+++ b/lib/tst_safe_file_at.c
@@ -35,13 +35,16 @@  const char *tst_decode_fd(const int fd)
 int safe_openat(const char *const file, const int lineno,
 		const int dirfd, const char *const path, const int oflags, ...)
 {
-	va_list ap;
 	int fd;
-	mode_t mode;
+	mode_t mode = 0;
 
-	va_start(ap, oflags);
-	mode = va_arg(ap, int);
-	va_end(ap);
+	if (__OPEN_NEEDS_MODE(oflags)) {
+		va_list ap;
+
+		va_start(ap, oflags);
+		mode = va_arg(ap, int);
+		va_end(ap);
+	}
 
 	fd = openat(dirfd, path, oflags, mode);
 	if (fd > -1)