diff mbox series

[v2,1/3] safe_open: Fix undefined behaviour in vararg handling

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

Commit Message

Tudor Cretu Nov. 29, 2022, 1:03 p.m. UTC 
Accessing elements in an empty va_list is undefined behaviour.
Therefore, remove the variadicness from safe_open as it always calls
open with the mode argument included.

Adapt the SAFE_OPEN macro to handle the change by passing a default
argument of 0 to mode if it's omitted.

Some instances of SAFE_OPEN were passing NULL as the mode argument
instead of an int. Remove it and allow the macros to handle it to avoid
a pointer to int conversion.

Signed-off-by: Tudor Cretu <tudor.cretu@arm.com>
---
 include/old/safe_macros.h                         |  6 ++++--
 include/safe_macros_fn.h                          |  3 ++-
 include/tst_safe_macros.h                         |  6 ++++--
 lib/safe_macros.c                                 | 13 +------------
 testcases/kernel/syscalls/fgetxattr/fgetxattr01.c |  2 +-
 testcases/kernel/syscalls/fgetxattr/fgetxattr02.c |  2 +-
 testcases/kernel/syscalls/fgetxattr/fgetxattr03.c |  2 +-
 testcases/kernel/syscalls/fsetxattr/fsetxattr01.c |  2 +-
 testcases/kernel/syscalls/fsetxattr/fsetxattr02.c |  2 +-
 9 files changed, 16 insertions(+), 22 deletions(-)
diff mbox series

Patch

diff --git a/include/old/safe_macros.h b/include/old/safe_macros.h
index fb1d7a110..d16540d63 100644
--- a/include/old/safe_macros.h
+++ b/include/old/safe_macros.h
@@ -59,9 +59,11 @@ 
 #define SAFE_MUNMAP(cleanup_fn, addr, length)	\
 	safe_munmap(__FILE__, __LINE__, (cleanup_fn), (addr), (length))
 
+#define __SAFE_OPEN(cleanup_fn, pathname, oflags, mode, ...)	\
+	safe_open(__FILE__, __LINE__, (cleanup_fn), (pathname), (oflags), (mode))
+
 #define SAFE_OPEN(cleanup_fn, pathname, oflags, ...)	\
-	safe_open(__FILE__, __LINE__, (cleanup_fn), (pathname), (oflags), \
-	    ##__VA_ARGS__)
+	__SAFE_OPEN((cleanup_fn), (pathname), (oflags), ##__VA_ARGS__, 0)
 
 #define SAFE_PIPE(cleanup_fn, fildes)	\
 	safe_pipe(__FILE__, __LINE__, cleanup_fn, (fildes))
diff --git a/include/safe_macros_fn.h b/include/safe_macros_fn.h
index 114d8fd43..d143079c3 100644
--- a/include/safe_macros_fn.h
+++ b/include/safe_macros_fn.h
@@ -74,7 +74,8 @@  int safe_munmap(const char *file, const int lineno,
                 void (*cleanup_fn)(void), void *addr, size_t length);
 
 int safe_open(const char *file, const int lineno,
-              void (*cleanup_fn)(void), const char *pathname, int oflags, ...);
+              void (*cleanup_fn)(void), const char *pathname, int oflags,
+              mode_t mode);
 
 int safe_pipe(const char *file, const int lineno,
               void (*cleanup_fn)(void), int fildes[2]);
diff --git a/include/tst_safe_macros.h b/include/tst_safe_macros.h
index 81c4b0844..d53555c88 100644
--- a/include/tst_safe_macros.h
+++ b/include/tst_safe_macros.h
@@ -86,9 +86,11 @@  void *safe_realloc(const char *file, const int lineno, void *ptr, size_t size);
 #define SAFE_MUNMAP(addr, length) \
 	safe_munmap(__FILE__, __LINE__, NULL, (addr), (length))
 
+#define __SAFE_OPEN(pathname, oflags, mode, ...) \
+	safe_open(__FILE__, __LINE__, NULL, (pathname), (oflags), (mode))
+
 #define SAFE_OPEN(pathname, oflags, ...) \
-	safe_open(__FILE__, __LINE__, NULL, (pathname), (oflags), \
-	    ##__VA_ARGS__)
+	__SAFE_OPEN((pathname), (oflags), ##__VA_ARGS__, 0)
 
 #define SAFE_PIPE(fildes) \
 	safe_pipe(__FILE__, __LINE__, NULL, (fildes))
diff --git a/lib/safe_macros.c b/lib/safe_macros.c
index d8816631f..a92b58347 100644
--- a/lib/safe_macros.c
+++ b/lib/safe_macros.c
@@ -234,20 +234,9 @@  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, ...)
+              const char *pathname, int oflags, mode_t mode)
 {
-	va_list ap;
 	int rval;
-	mode_t mode;
-
-	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);
 
 	rval = open(pathname, oflags, mode);
 
diff --git a/testcases/kernel/syscalls/fgetxattr/fgetxattr01.c b/testcases/kernel/syscalls/fgetxattr/fgetxattr01.c
index 35c46a1c3..52e6e44ab 100644
--- a/testcases/kernel/syscalls/fgetxattr/fgetxattr01.c
+++ b/testcases/kernel/syscalls/fgetxattr/fgetxattr01.c
@@ -111,7 +111,7 @@  static void setup(void)
 	size_t i = 0;
 
 	SAFE_TOUCH(FNAME, 0644, NULL);
-	fd = SAFE_OPEN(FNAME, O_RDONLY, NULL);
+	fd = SAFE_OPEN(FNAME, O_RDONLY);
 
 	for (i = 0; i < ARRAY_SIZE(tc); i++) {
 		tc[i].value = SAFE_MALLOC(tc[i].size);
diff --git a/testcases/kernel/syscalls/fgetxattr/fgetxattr02.c b/testcases/kernel/syscalls/fgetxattr/fgetxattr02.c
index 82fb676be..c3cff0aab 100644
--- a/testcases/kernel/syscalls/fgetxattr/fgetxattr02.c
+++ b/testcases/kernel/syscalls/fgetxattr/fgetxattr02.c
@@ -244,7 +244,7 @@  static void setup(void)
 			SAFE_BIND(tc[i].fd, (const struct sockaddr *) &sun,
 					sizeof(struct sockaddr_un));
 		} else {
-			tc[i].fd = SAFE_OPEN(tc[i].fname, tc[i].fflags, NULL);
+			tc[i].fd = SAFE_OPEN(tc[i].fname, tc[i].fflags);
 		}
 
 		if (tc[i].exp_ret >= 0) {
diff --git a/testcases/kernel/syscalls/fgetxattr/fgetxattr03.c b/testcases/kernel/syscalls/fgetxattr/fgetxattr03.c
index d293ffca9..0581b9670 100644
--- a/testcases/kernel/syscalls/fgetxattr/fgetxattr03.c
+++ b/testcases/kernel/syscalls/fgetxattr/fgetxattr03.c
@@ -48,7 +48,7 @@  static void verify_fgetxattr(void)
 static void setup(void)
 {
 	SAFE_TOUCH(FILENAME, 0644, NULL);
-	fd = SAFE_OPEN(FILENAME, O_RDONLY, NULL);
+	fd = SAFE_OPEN(FILENAME, O_RDONLY);
 
 	SAFE_FSETXATTR(fd, XATTR_TEST_KEY, XATTR_TEST_VALUE,
 			XATTR_TEST_VALUE_SIZE, XATTR_CREATE);
diff --git a/testcases/kernel/syscalls/fsetxattr/fsetxattr01.c b/testcases/kernel/syscalls/fsetxattr/fsetxattr01.c
index ffec8104f..d799e477f 100644
--- a/testcases/kernel/syscalls/fsetxattr/fsetxattr01.c
+++ b/testcases/kernel/syscalls/fsetxattr/fsetxattr01.c
@@ -205,7 +205,7 @@  static void setup(void)
 	long_value[XATTR_SIZE_MAX + 1] = '\0';
 
 	SAFE_TOUCH(FNAME, 0644, NULL);
-	fd = SAFE_OPEN(FNAME, O_RDONLY, NULL);
+	fd = SAFE_OPEN(FNAME, O_RDONLY);
 
 	for (i = 0; i < ARRAY_SIZE(tc); i++) {
 		if (!tc[i].key)
diff --git a/testcases/kernel/syscalls/fsetxattr/fsetxattr02.c b/testcases/kernel/syscalls/fsetxattr/fsetxattr02.c
index 3aea4b59e..0336c964a 100644
--- a/testcases/kernel/syscalls/fsetxattr/fsetxattr02.c
+++ b/testcases/kernel/syscalls/fsetxattr/fsetxattr02.c
@@ -211,7 +211,7 @@  static void setup(void)
 	for (i = 0; i < ARRAY_SIZE(tc); i++) {
 
 		if (!tc[i].issocket) {
-			tc[i].fd = SAFE_OPEN(tc[i].fname, tc[i].fflags, NULL);
+			tc[i].fd = SAFE_OPEN(tc[i].fname, tc[i].fflags);
 			continue;
 		}