On 01/11/2022 13:27, 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
A review branch is created at: https://git.morello-project.org/tudcre01/morello-linux-ltp/-/tree/review/saf...
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..854c087bc 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__)
Shouldn't the macro arguments here be protected by parentheses, as in the macro above ? If so, it is needed as well in two other places below.
#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..ca51755a6 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__)
Might need parentheses as well.
#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..9fd51c3ea 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__)
And here as well.
#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)
Other than the parentheses for the macro arguments, everything looks good !
I had to do a bit of checking on my side regarding the use of `_SAFE_OPEN_HELP` as I'm not very good with macros, but it seems to work as expected an select the correct overload :)
It feels like this patch could be upstreamed as well ?
Regards, Téo