api: provide 32-bit friendly argument comparison macros#140
api: provide 32-bit friendly argument comparison macros#140michaelweiser wants to merge 1 commit intoseccomp:masterfrom michaelweiser:signextend
Conversation
include/seccomp.h.in
Outdated
| */ | ||
| #define SCMP_SYS(x) (__NR_##x) | ||
|
|
||
| /* Helper macros for the 32-bit argument comparison macros */ |
There was a problem hiding this comment.
I would suggest adding a warning that these macros should not be used directly, for example:
/* Helpers for the argument comparison macros, DO NOT USE directly */
include/seccomp.h.in
Outdated
| #define SCMP_SYS(x) (__NR_##x) | ||
|
|
||
| /* Helper macros for the 32-bit argument comparison macros */ | ||
| /* Expand to the number of arguments given */ |
There was a problem hiding this comment.
You can probably just drop this comment. Since we aren't intending this for use by callers, some "mystery" might be good :)
include/seccomp.h.in
Outdated
| #define SCMP_VA_NUM_ARGS(...) SCMP_VA_NUM_ARGS_IMPL(__VA_ARGS__,2,1) | ||
| #define SCMP_VA_NUM_ARGS_IMPL(_1,_2,N,...) N | ||
|
|
||
| /* Expand to alternative names based on the number of arguments given. */ |
There was a problem hiding this comment.
I'd drop this comment too and move this block up so there is no vertical whitespace between the SCMP_VA_* macros and the SCMP_MACRO_DISPATCHER_* macros.
include/seccomp.h.in
Outdated
| SCMP_MACRO_DISPATCHER_IMPL2(func, nargs) | ||
| #define SCMP_MACRO_DISPATCHER_IMPL2(func, nargs) \ | ||
| func ## nargs | ||
|
|
There was a problem hiding this comment.
I wonder if we should preface all of these "DO NOT USE!" macros with an underscore to further indicate that they are special. For example, _SCMP_VA_NUM_ARGS() and _SCMP_MACRO_DISPATCHER().
include/seccomp.h.in
Outdated
|
|
||
| /* Implement casting of operands to uint32_t */ | ||
| #define SCMP_CMP32_1(x, y, z) SCMP_CMP64(x, y, (uint32_t)(z)) | ||
| #define SCMP_CMP32_2(x, y, z, q) SCMP_CMP64(x, y, (uint32_t)(z), (uint32_t)(q)) |
There was a problem hiding this comment.
These two macros are borderline, but I'm leaning towards treating them like the others: drop the comment, add the leading underscore, drop the vertical whitespace.
tests/49-sim-static_init.c
Outdated
| if (ctx == NULL) | ||
| return ENOMEM; | ||
|
|
||
| for (a = f; a->syscall != 0; a++) { |
There was a problem hiding this comment.
The important bit of this test is to verify the macros as suitable initializers, so I think we can drop the for loop and just test one syscall.
There was a problem hiding this comment.
Ooof. Sorry. Scratch the above comment. I think I might be a little too tired to review this properly :)
tests/49-sim-static_init.c
Outdated
| goto out; | ||
| } | ||
|
|
||
| rc = util_filter_output(&opts, ctx); |
There was a problem hiding this comment.
In general I think we can fold this test into test 48, just use syscalls 2000/2032/2064. And don't forget to add your name to the top :)
|
Thanks a lot @michaelweiser for putting this together. I've added a few inline comments to the PR, but in general it looks pretty good. Also, I'm sure you've seen it by now, but Travis is failing on the new test. We obviously need to fix that too. |
We have a longstanding issue with 32-bit to 64-bit sign extension inadvertently resulting in bogus syscall argument extensions. This patch introduces a new set of argument comparison macros which limit the argument values to 32-bit values so that we don't run into problems with sign extension. We use the macro overloading proposed by Roman at https://kecher.net/overloading-macros/ to retain the feature of these macros being usable as static initializers. Thanks to @jdstrand on GitHub for reporting the problem. Signed-off-by: Paul Moore <paul@paul-moore.com> Signed-off-by: Michael Weiser <michael.weiser@gmx.de>
|
Oookay, this should do it. I've incorporated all your suggestions and found out that I need to make the python test match the c native one. I have no idea how to approximate the static initializer part in python and have just duplicated the code with an appropriate comment. |
|
Merged via 80a987d, thanks a lot for the help @michaelweiser! I did make a small 80-char line width change in include/seccomp.h.in, but that was purely superficial. |
2 participants
As per discussion in #69 here's the adjusted patch introducing 32bit-specific comparison macros. I've added a clone of test 48 that only differs in that it uses the macros to intialise an array of structs to see if that even compiles and actually works.
We have a longstanding issue with 32-bit to 64-bit sign extension
inadvertently resulting in bogus syscall argument extensions. This
patch introduces a new set of argument comparison macros which
limit the argument values to 32-bit values so that we don't run into
problems with sign extension.
We use the macro overloading proposed by Roman at
https://kecher.net/overloading-macros/ to retain the feature of these
macros being usable as static initializers.
Thanks to @jdstrand on GitHub for reporting the problem.
Signed-off-by: Paul Moore paul@paul-moore.com
Signed-off-by: Michael Weiser michael.weiser@gmx.de