Accessing elements in an empty va_list is undefined behaviour. Therefore, remove the variadicness from safe_* wrappers that always call the functions with the optional argument included.
Adapt the SAFE_OPEN, SAFE_OPENAT, SAFE_SEMCTL macros to handle the change by passing a default arguments if they're omitted.
v5: - Simplified the SAFE_ macros - Removed the SAFE_COND_HANDLER
v4: - Updated the commit message of Patch 1 and some comments - Renamed safe_vararg_macros.h -> tst_safe_vararg_macros.h - Renamed SAFE_WRAPPER_COND_HANDLER -> SAFE_COND_HANDLER - Improved alignment to make macros more readable
v3: - Combined the open/openat patch with the semctl one into a single series. - Split the open/openat patch into two patches. - Introduced a new PATCH (i.e. 1/4) that implements a common handler chooser for open/openat/semctl cases. - Removed an unused va_list variable.
v2: - Added parenthesis around macro arguments
Review branch: https://git.morello-project.org/tudcre01/morello-linux-ltp/-/commits/review/...
Tudor Cretu (3): safe_open: Fix undefined behaviour in vararg handling safe_openat: Fix undefined behaviour in vararg handling safe_semctl: Fix undefined behaviour in vararg handling
include/old/safe_macros.h | 6 ++++-- include/safe_macros_fn.h | 3 ++- include/tst_safe_file_at.h | 10 ++++++---- include/tst_safe_macros.h | 6 ++++-- include/tst_safe_sysv_ipc.h | 14 +++++++++----- lib/safe_macros.c | 13 +------------ lib/tst_safe_file_at.c | 11 +++-------- lib/tst_safe_sysv_ipc.c | 10 +--------- 8 files changed, 30 insertions(+), 43 deletions(-)
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.
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 +------------ 4 files changed, 11 insertions(+), 17 deletions(-)
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 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_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 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);
Accessing elements in an empty va_list is undefined behaviour. Therefore, remove the variadicness from safe_openat as it always calls openat with the mode argument included.
Adapt the SAFE_OPENAT macro 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 --- include/tst_safe_file_at.h | 10 ++++++---- lib/tst_safe_file_at.c | 11 +++-------- 2 files changed, 9 insertions(+), 12 deletions(-)
diff --git a/include/tst_safe_file_at.h b/include/tst_safe_file_at.h index 8df34227f..b8e907291 100644 --- a/include/tst_safe_file_at.h +++ b/include/tst_safe_file_at.h @@ -9,9 +9,11 @@ #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(dirfd, path, oflags, mode, ...) \ + safe_openat(__FILE__, __LINE__, (dirfd), (path), (oflags), (mode)) + +#define SAFE_OPENAT(dirfd, path, oflags, ...) \ + __SAFE_OPENAT((dirfd), (path), (oflags), ##__VA_ARGS__, 0)
#define SAFE_FILE_READAT(dirfd, path, buf, nbyte) \ safe_file_readat(__FILE__, __LINE__, \ @@ -29,7 +31,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) __attribute__((nonnull, warn_unused_result));
ssize_t safe_file_readat(const char *const file, const int lineno, diff --git a/lib/tst_safe_file_at.c b/lib/tst_safe_file_at.c index ca8ef2f68..9582f17d8 100644 --- a/lib/tst_safe_file_at.c +++ b/lib/tst_safe_file_at.c @@ -33,15 +33,10 @@ 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) { - va_list ap; 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 +53,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)
Accessing elements in an empty va_list is undefined behaviour. Therefore, remove the variadicness from safe_semctl as it always calls semctl with the union semun argument included.
Adapt the SAFE_SEMCTL macro to handle the change by passing a zero-initialised union semun if it's omitted.
Signed-off-by: Tudor Cretu tudor.cretu@arm.com --- include/tst_safe_sysv_ipc.h | 14 +++++++++----- lib/tst_safe_sysv_ipc.c | 10 +--------- 2 files changed, 10 insertions(+), 14 deletions(-)
diff --git a/include/tst_safe_sysv_ipc.h b/include/tst_safe_sysv_ipc.h index 7804ce192..925ae8186 100644 --- a/include/tst_safe_sysv_ipc.h +++ b/include/tst_safe_sysv_ipc.h @@ -10,6 +10,7 @@ #include <sys/msg.h> #include <sys/shm.h> #include <sys/sem.h> +#include "lapi/sem.h"
int safe_msgget(const char *file, const int lineno, key_t key, int msgflg); #define SAFE_MSGGET(key, msgflg) \ @@ -58,11 +59,14 @@ int safe_semget(const char *file, const int lineno, key_t key, int nsems, safe_semget(__FILE__, __LINE__, (key), (nsems), (semflg))
int safe_semctl(const char *file, const int lineno, int semid, int semnum, - int cmd, ...); -#define SAFE_SEMCTL(semid, semnum, cmd, ...) ({ \ - int tst_ret_ = safe_semctl(__FILE__, __LINE__, (semid), (semnum), \ - (cmd), ##__VA_ARGS__); \ - (semid) = ((cmd) == IPC_RMID ? -1 : (semid)); \ + int cmd, union semun un); +#define __SAFE_SEMCTL(semid, semnum, cmd, un, ...) \ + safe_semctl(__FILE__, __LINE__, (semid), (semnum), (cmd), (un)) + +#define SAFE_SEMCTL(semid, semnum, cmd, ...) ({ \ + int tst_ret_ = __SAFE_SEMCTL((semid), (semnum), (cmd), ##__VA_ARGS__, \ + (union semun){0}); \ + (semid) = ((cmd) == IPC_RMID ? -1 : (semid)); \ tst_ret_; })
int safe_semop(const char *file, const int lineno, int semid, struct sembuf *sops, diff --git a/lib/tst_safe_sysv_ipc.c b/lib/tst_safe_sysv_ipc.c index 5eaa82539..f99f6db5e 100644 --- a/lib/tst_safe_sysv_ipc.c +++ b/lib/tst_safe_sysv_ipc.c @@ -228,17 +228,9 @@ int safe_semget(const char *file, const int lineno, key_t key, int nsems, }
int safe_semctl(const char *file, const int lineno, int semid, int semnum, - int cmd, ...) + int cmd, union semun un) { int rval; - va_list va; - union semun un; - - va_start(va, cmd); - - un = va_arg(va, union semun); - - va_end(va);
rval = semctl(semid, semnum, cmd, un);
On 21/11/2022 16:16, Tudor Cretu wrote:
Accessing elements in an empty va_list is undefined behaviour. Therefore, remove the variadicness from safe_semctl as it always calls semctl with the union semun argument included.
Adapt the SAFE_SEMCTL macro to handle the change by passing a zero-initialised union semun if it's omitted.
Signed-off-by: Tudor Cretu tudor.cretu@arm.com
include/tst_safe_sysv_ipc.h | 14 +++++++++----- lib/tst_safe_sysv_ipc.c | 10 +--------- 2 files changed, 10 insertions(+), 14 deletions(-)
diff --git a/include/tst_safe_sysv_ipc.h b/include/tst_safe_sysv_ipc.h index 7804ce192..925ae8186 100644 --- a/include/tst_safe_sysv_ipc.h +++ b/include/tst_safe_sysv_ipc.h @@ -10,6 +10,7 @@ #include <sys/msg.h> #include <sys/shm.h> #include <sys/sem.h> +#include "lapi/sem.h" int safe_msgget(const char *file, const int lineno, key_t key, int msgflg); #define SAFE_MSGGET(key, msgflg) \ @@ -58,11 +59,14 @@ int safe_semget(const char *file, const int lineno, key_t key, int nsems, safe_semget(__FILE__, __LINE__, (key), (nsems), (semflg)) int safe_semctl(const char *file, const int lineno, int semid, int semnum,
int cmd, ...);-#define SAFE_SEMCTL(semid, semnum, cmd, ...) ({ \
- int tst_ret_ = safe_semctl(__FILE__, __LINE__, (semid), (semnum), \
(cmd), ##__VA_ARGS__); \- (semid) = ((cmd) == IPC_RMID ? -1 : (semid)); \
int cmd, union semun un);+#define __SAFE_SEMCTL(semid, semnum, cmd, un, ...) \
- safe_semctl(__FILE__, __LINE__, (semid), (semnum), (cmd), (un))
+#define SAFE_SEMCTL(semid, semnum, cmd, ...) ({ \
- int tst_ret_ = __SAFE_SEMCTL((semid), (semnum), (cmd), ##__VA_ARGS__, \
(union semun){0}); \- (semid) = ((cmd) == IPC_RMID ? -1 : (semid)); \
Nit: it's preferable to align the \ using hard tabs only. It's not really an issue if that goes above 80 char.
Apart from that minor thing, it all looks good to me!
Kevin
tst_ret_; }) int safe_semop(const char *file, const int lineno, int semid, struct sembuf *sops, diff --git a/lib/tst_safe_sysv_ipc.c b/lib/tst_safe_sysv_ipc.c index 5eaa82539..f99f6db5e 100644 --- a/lib/tst_safe_sysv_ipc.c +++ b/lib/tst_safe_sysv_ipc.c @@ -228,17 +228,9 @@ int safe_semget(const char *file, const int lineno, key_t key, int nsems, } int safe_semctl(const char *file, const int lineno, int semid, int semnum,
int cmd, ...)
int cmd, union semun un){ int rval;
- va_list va;
- union semun un;
- va_start(va, cmd);
- un = va_arg(va, union semun);
- va_end(va);
rval = semctl(semid, semnum, cmd, un);
On Mon, Nov 21, 2022 at 05:27:47PM +0100, Kevin Brodsky wrote:
On 21/11/2022 16:16, Tudor Cretu wrote:
Accessing elements in an empty va_list is undefined behaviour. Therefore, remove the variadicness from safe_semctl as it always calls semctl with the union semun argument included.
Adapt the SAFE_SEMCTL macro to handle the change by passing a zero-initialised union semun if it's omitted.
Signed-off-by: Tudor Cretu tudor.cretu@arm.com
include/tst_safe_sysv_ipc.h | 14 +++++++++----- lib/tst_safe_sysv_ipc.c | 10 +--------- 2 files changed, 10 insertions(+), 14 deletions(-)
diff --git a/include/tst_safe_sysv_ipc.h b/include/tst_safe_sysv_ipc.h index 7804ce192..925ae8186 100644 --- a/include/tst_safe_sysv_ipc.h +++ b/include/tst_safe_sysv_ipc.h @@ -10,6 +10,7 @@ #include <sys/msg.h> #include <sys/shm.h> #include <sys/sem.h> +#include "lapi/sem.h" int safe_msgget(const char *file, const int lineno, key_t key, int msgflg); #define SAFE_MSGGET(key, msgflg) \ @@ -58,11 +59,14 @@ int safe_semget(const char *file, const int lineno, key_t key, int nsems, safe_semget(__FILE__, __LINE__, (key), (nsems), (semflg)) int safe_semctl(const char *file, const int lineno, int semid, int semnum,
int cmd, ...);-#define SAFE_SEMCTL(semid, semnum, cmd, ...) ({ \
- int tst_ret_ = safe_semctl(__FILE__, __LINE__, (semid), (semnum), \
(cmd), ##__VA_ARGS__); \- (semid) = ((cmd) == IPC_RMID ? -1 : (semid)); \
int cmd, union semun un);+#define __SAFE_SEMCTL(semid, semnum, cmd, un, ...) \
- safe_semctl(__FILE__, __LINE__, (semid), (semnum), (cmd), (un))
+#define SAFE_SEMCTL(semid, semnum, cmd, ...) ({ \
- int tst_ret_ = __SAFE_SEMCTL((semid), (semnum), (cmd), ##__VA_ARGS__, \
(union semun){0}); \- (semid) = ((cmd) == IPC_RMID ? -1 : (semid)); \
Nit: it's preferable to align the \ using hard tabs only. It's not really an issue if that goes above 80 char.
I can fix those when applying the patches as long ad @Tudor is ok with that.
--- BR B.
Apart from that minor thing, it all looks good to me!
Kevin
tst_ret_; }) int safe_semop(const char *file, const int lineno, int semid, struct sembuf *sops, diff --git a/lib/tst_safe_sysv_ipc.c b/lib/tst_safe_sysv_ipc.c index 5eaa82539..f99f6db5e 100644 --- a/lib/tst_safe_sysv_ipc.c +++ b/lib/tst_safe_sysv_ipc.c @@ -228,17 +228,9 @@ int safe_semget(const char *file, const int lineno, key_t key, int nsems, } int safe_semctl(const char *file, const int lineno, int semid, int semnum,
int cmd, ...)
int cmd, union semun un){ int rval;
- va_list va;
- union semun un;
- va_start(va, cmd);
- un = va_arg(va, union semun);
- va_end(va);
rval = semctl(semid, semnum, cmd, un);
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
On 21-11-2022 16:43, Beata Michalska wrote:
On Mon, Nov 21, 2022 at 05:27:47PM +0100, Kevin Brodsky wrote:
On 21/11/2022 16:16, Tudor Cretu wrote:
Accessing elements in an empty va_list is undefined behaviour. Therefore, remove the variadicness from safe_semctl as it always calls semctl with the union semun argument included.
Adapt the SAFE_SEMCTL macro to handle the change by passing a zero-initialised union semun if it's omitted.
Signed-off-by: Tudor Cretu tudor.cretu@arm.com
include/tst_safe_sysv_ipc.h | 14 +++++++++----- lib/tst_safe_sysv_ipc.c | 10 +--------- 2 files changed, 10 insertions(+), 14 deletions(-)
diff --git a/include/tst_safe_sysv_ipc.h b/include/tst_safe_sysv_ipc.h index 7804ce192..925ae8186 100644 --- a/include/tst_safe_sysv_ipc.h +++ b/include/tst_safe_sysv_ipc.h @@ -10,6 +10,7 @@ #include <sys/msg.h> #include <sys/shm.h> #include <sys/sem.h> +#include "lapi/sem.h" int safe_msgget(const char *file, const int lineno, key_t key, int msgflg); #define SAFE_MSGGET(key, msgflg) \ @@ -58,11 +59,14 @@ int safe_semget(const char *file, const int lineno, key_t key, int nsems, safe_semget(__FILE__, __LINE__, (key), (nsems), (semflg)) int safe_semctl(const char *file, const int lineno, int semid, int semnum,
int cmd, ...);-#define SAFE_SEMCTL(semid, semnum, cmd, ...) ({ \
- int tst_ret_ = safe_semctl(__FILE__, __LINE__, (semid), (semnum), \
(cmd), ##__VA_ARGS__); \- (semid) = ((cmd) == IPC_RMID ? -1 : (semid)); \
int cmd, union semun un);+#define __SAFE_SEMCTL(semid, semnum, cmd, un, ...) \
- safe_semctl(__FILE__, __LINE__, (semid), (semnum), (cmd), (un))
+#define SAFE_SEMCTL(semid, semnum, cmd, ...) ({ \
- int tst_ret_ = __SAFE_SEMCTL((semid), (semnum), (cmd), ##__VA_ARGS__, \
(union semun){0}); \- (semid) = ((cmd) == IPC_RMID ? -1 : (semid)); \
Nit: it's preferable to align the \ using hard tabs only. It's not really an issue if that goes above 80 char.
I can fix those when applying the patches as long ad @Tudor is ok with that.
Many thanks for that, sure go ahead :).
Thanks, Tudor
BR B.
Apart from that minor thing, it all looks good to me!
Kevin
tst_ret_; }) int safe_semop(const char *file, const int lineno, int semid, struct sembuf *sops, diff --git a/lib/tst_safe_sysv_ipc.c b/lib/tst_safe_sysv_ipc.c index 5eaa82539..f99f6db5e 100644 --- a/lib/tst_safe_sysv_ipc.c +++ b/lib/tst_safe_sysv_ipc.c @@ -228,17 +228,9 @@ int safe_semget(const char *file, const int lineno, key_t key, int nsems, } int safe_semctl(const char *file, const int lineno, int semid, int semnum,
int cmd, ...)
int cmd, union semun un)
- va_list va;
- union semun un;
- va_start(va, cmd);
- un = va_arg(va, union semun);
- va_end(va);
rval = semctl(semid, semnum, cmd, un);
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
Just landed on _next_. Thanks for the series!
--- BR B.
On Mon, Nov 21, 2022 at 03:16:08PM +0000, Tudor Cretu wrote:
Accessing elements in an empty va_list is undefined behaviour. Therefore, remove the variadicness from safe_* wrappers that always call the functions with the optional argument included.
Adapt the SAFE_OPEN, SAFE_OPENAT, SAFE_SEMCTL macros to handle the change by passing a default arguments if they're omitted.
v5:
- Simplified the SAFE_ macros
- Removed the SAFE_COND_HANDLER
v4:
- Updated the commit message of Patch 1 and some comments
- Renamed safe_vararg_macros.h -> tst_safe_vararg_macros.h
- Renamed SAFE_WRAPPER_COND_HANDLER -> SAFE_COND_HANDLER
- Improved alignment to make macros more readable
v3:
- Combined the open/openat patch with the semctl one into a single series.
- Split the open/openat patch into two patches.
- Introduced a new PATCH (i.e. 1/4) that implements a common handler chooser for open/openat/semctl cases.
- Removed an unused va_list variable.
v2:
- Added parenthesis around macro arguments
Review branch: https://git.morello-project.org/tudcre01/morello-linux-ltp/-/commits/review/...
Tudor Cretu (3): safe_open: Fix undefined behaviour in vararg handling safe_openat: Fix undefined behaviour in vararg handling safe_semctl: Fix undefined behaviour in vararg handling
include/old/safe_macros.h | 6 ++++-- include/safe_macros_fn.h | 3 ++- include/tst_safe_file_at.h | 10 ++++++---- include/tst_safe_macros.h | 6 ++++-- include/tst_safe_sysv_ipc.h | 14 +++++++++----- lib/safe_macros.c | 13 +------------ lib/tst_safe_file_at.c | 11 +++-------- lib/tst_safe_sysv_ipc.c | 10 +--------- 8 files changed, 30 insertions(+), 43 deletions(-)
-- 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
linux-morello-ltp@op-lists.linaro.org
-
Beata Michalska -
Kevin Brodsky -
Tudor Cretu