On 01/11/2022 16:04, Tudor Cretu wrote:
Accessing elements in an empty va_list is undefined behaviour. Therefore, remove the variadicness from safe_open and safe_openat as they always call open/openat with the mode argument included.
Adapt the SAFE_OPEN and SAFE_OPENAT macros to handle the change by passing a default argument of 0 to mode if it's omitted.
Signed-off-by: Tudor Cretu tudor.cretu@arm.com
Looks good to me !
It's a bit weird that in some places it handles indentation of function arguments with only spaces and with tabs in others, but this comes from the original file and not your patch.
Regards, Téo
v1..v2:
- Added parenthesis around macro arguments
A review branch has been created here: https://git.morello-project.org/tudcre01/morello-linux-ltp/-/commits/review/...
include/old/safe_macros.h | 13 +++++++++++-- include/safe_macros_fn.h | 3 ++- include/tst_safe_file_at.h | 18 ++++++++++++++---- include/tst_safe_macros.h | 12 ++++++++++-- lib/safe_macros.c | 13 +------------ lib/tst_safe_file_at.c | 10 +++------- 6 files changed, 41 insertions(+), 28 deletions(-)
diff --git a/include/old/safe_macros.h b/include/old/safe_macros.h index fb1d7a110..34070b1d4 100644 --- a/include/old/safe_macros.h +++ b/include/old/safe_macros.h @@ -59,9 +59,18 @@ #define SAFE_MUNMAP(cleanup_fn, addr, length) \ safe_munmap(__FILE__, __LINE__, (cleanup_fn), (addr), (length)) -#define SAFE_OPEN(cleanup_fn, pathname, oflags, ...) \ +#define _SAFE_OPEN_3(cleanup_fn, pathname, oflags) \
- safe_open(__FILE__, __LINE__, (cleanup_fn), (pathname), (oflags), 0)
+#define _SAFE_OPEN_4(cleanup_fn, pathname, oflags, mode) \ safe_open(__FILE__, __LINE__, (cleanup_fn), (pathname), (oflags), \
##__VA_ARGS__)
(mode))+#define _SAFE_OPEN_HELPER(_0, _1, _SAFE_OPEN_X, ...) _SAFE_OPEN_X
+#define SAFE_OPEN(cleanup_fn, pathname, oflags, ...) \
- _SAFE_OPEN_HELPER(, ##__VA_ARGS__, _SAFE_OPEN_4, _SAFE_OPEN_3) \
((cleanup_fn), (pathname), (oflags), ##__VA_ARGS__)#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 3df952811..e84759839 100644 --- a/include/safe_macros_fn.h +++ b/include/safe_macros_fn.h @@ -62,7 +62,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_file_at.h b/include/tst_safe_file_at.h index 8df34227f..94232bd32 100644 --- a/include/tst_safe_file_at.h +++ b/include/tst_safe_file_at.h @@ -9,9 +9,19 @@ #include <sys/types.h> #include <stdarg.h> -#define SAFE_OPENAT(dirfd, path, oflags, ...) \
- safe_openat(__FILE__, __LINE__, \
(dirfd), (path), (oflags), ## __VA_ARGS__)+#define _SAFE_OPENAT_3(dirfd, path, oflags) \
- safe_openat(__FILE__, __LINE__, \
(dirfd), (path), (oflags), 0)+#define _SAFE_OPENAT_4(dirfd, path, oflags, mode) \
- safe_openat(__FILE__, __LINE__, \
(dirfd), (path), (oflags), (mode))+#define _SAFE_OPENAT_HELPER(_0, _1, _SAFE_OPENAT_X, ...) _SAFE_OPENAT_X
+#define SAFE_OPENAT(dirfd, path, oflags, ...) \
- _SAFE_OPENAT_HELPER(, ##__VA_ARGS__, _SAFE_OPENAT_4, _SAFE_OPENAT_3) \
- ((dirfd), (path), (oflags), ##__VA_ARGS__)
#define SAFE_FILE_READAT(dirfd, path, buf, nbyte) \ safe_file_readat(__FILE__, __LINE__, \ @@ -29,7 +39,7 @@ const char *tst_decode_fd(const int fd) __attribute__((warn_unused_result)); int safe_openat(const char *const file, const int lineno, const int dirfd,
const char *const path, const int oflags, ...)
const char *const path, const int oflags, const mode_t mode)ssize_t safe_file_readat(const char *const file, const int lineno, diff --git a/include/tst_safe_macros.h b/include/tst_safe_macros.h index 81c4b0844..cb18bb384 100644 --- a/include/tst_safe_macros.h +++ b/include/tst_safe_macros.h @@ -86,9 +86,17 @@ 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_2(pathname, oflags) \
- safe_open(__FILE__, __LINE__, NULL, (pathname), (oflags), 0)
+#define _SAFE_OPEN_3(pathname, oflags, mode) \
- safe_open(__FILE__, __LINE__, NULL, (pathname), (oflags), (mode))
+#define _SAFE_OPEN_HELPER(_0, _1, _SAFE_OPEN_X, ...) _SAFE_OPEN_X
- #define SAFE_OPEN(pathname, oflags, ...) \
- safe_open(__FILE__, __LINE__, NULL, (pathname), (oflags), \
##__VA_ARGS__)
- _SAFE_OPEN_HELPER(, ##__VA_ARGS__, _SAFE_OPEN_3, _SAFE_OPEN_2) \
- ((pathname), (oflags), ##__VA_ARGS__)
#define SAFE_PIPE(fildes) \ safe_pipe(__FILE__, __LINE__, NULL, (fildes)) diff --git a/lib/safe_macros.c b/lib/safe_macros.c index a5b6bc504..40891e841 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/lib/tst_safe_file_at.c b/lib/tst_safe_file_at.c index ca8ef2f68..1c1f646bc 100644 --- a/lib/tst_safe_file_at.c +++ b/lib/tst_safe_file_at.c @@ -33,15 +33,11 @@ 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, ...)
const int dirfd, const char *const path, const int oflags,const mode_t mode)
- mode_t mode;
- va_start(ap, oflags);
- mode = va_arg(ap, int);
- va_end(ap);
fd = openat(dirfd, path, oflags, mode); if (fd > -1) @@ -58,7 +54,7 @@ ssize_t safe_file_readat(const char *const file, const int lineno, const int dirfd, const char *const path, char *const buf, const size_t nbyte) {
- int fd = safe_openat(file, lineno, dirfd, path, O_RDONLY);
- int fd = safe_openat(file, lineno, dirfd, path, O_RDONLY, 0); ssize_t rval;
if (fd < 0)