On 07-11-2022 12:46, Beata Michalska wrote:
On Tue, Nov 01, 2022 at 04:04:53PM +0000, 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
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
This is a pretty clever idea that exploits some of the macro specificities, good one! It's somehow not that readable for me (at least at the first glance, with the first two args naming and then alternate handlers which end up at a different position depending whether the first arg gonna get swallowed or not), but overall I like this one.
Thank you for the review! Do you have any suggestion for different naming? I'm happy to change them. If not, maybe I should just detail how it works in the commit message. Otherwise, I could add a comment, but I would need to replicate it everywhere I did this (i.e. 4 times).
+#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)I assume this one should be removed as well.
Oups, thank you! I need to check my compiler flags...
Thanks, Tudor
BR B.
int fd;
- 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) -- 2.25.1
linux-morello-ltp mailing list -- linux-morello-ltp@op-lists.linaro.org To unsubscribe send an email to linux-morello-ltp-leave@op-lists.linaro.org