On Mon, Nov 07, 2022 at 03:23:07PM +0000, Tudor Cretu wrote:
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).
I agree it is not ideal to comment (the same) on each single instance of it, and I am somewhat not entirely convinced commit message would be a good alternative either.
What if we could get single macro for selecting the right 'handler' that could be used across all those affected ones ? like:
#define SAFE_WRAPPER_COND_HANDLER(cond_empty_drop, cond_non_empty, SAFE_HELPER, ...)
or smth along the lines of that, and then have both, this patch, and the semctl one, reuse it as needed ? It seems to work the same way in all cases: shifting the args list by one if the __VA_ARGS__ is empty, provided that len(__VA_ARGS__) <= 1. This could be then properly documented.
--- B.
+#define SAFE_OPEN(cleanup_fn, pathname, oflags, ...) \
- _SAFE_OPEN_HELPER(, ##__VA_ARGS__, _SAFE_OPEN_4, _SAFE_OPEN_3) \
((cleanup_fn), (pathname), (oflags), ##__VA_ARGS__)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);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)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