Add metatests for the fork test helper as unit tests#737
Add metatests for the fork test helper as unit tests#737gilles-peskine-arm wants to merge 9 commits into
Conversation
gilles-peskine-arm
added
size-s
github-project-automation
Bot
moved this to In Development
in Roadmap pull requests (new board)
gilles-peskine-arm
force-pushed
the
fork-helper-unit-tests-create-crypto
branch
2 times, most recently
from
3f77a41 to
950aefa
Compare
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
`test_suite_metatest` is intended for tests of test code. We generally don't test the test code, but there are a few cases of test code that is especially complicated or fragile where it's good to have tests to ensure that failures are captured and are reported accurately. This complements the existing test program `metatest.c`. The test program can validate failure behavior that crashes the program (e.g. sanitizer ensuring a crash on an invalid pointer dereference) or that relies on exit-time checks (e.g. leak detector), but it's not good at asserting anything beyond the fact that a failure was detected. The unit test framework has a better structure for metatests that are shaped as a unit test with assertions, where we want to report failures of the metatest assertions accurately, but it can't allow crashes. Even though the code tested by `test_suite_metatest` is version-independent and located in the framework repository, we keep `test_suite_metatest` in a consuming branch, because we don't have any testing inside the framework, and we don't look for unit tests inside the framework. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm
force-pushed
the
fork-helper-unit-tests-create-crypto
branch
from
950aefa to
670e995
Compare
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Test what happens on plausible system failures: running out of resources to fork a process (memory, process table), to open a file (memory, file descriptor table) or to write to a file (disk space). Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm
force-pushed
the
fork-helper-unit-tests-create-crypto
branch
from
670e995 to
f2d48e8
Compare
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Ensure that the signal we're testing with isn't ignored. It might be masked though, depending on the surrounding infrastructure. Test with SIGTERM rather than SIGHUP: it's less likely to be masked. This fixes a failure of the SIGHUP test case on FreeBSD in our CI. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm
added
needs-review
bjwtaylor
left a comment
bjwtaylor
left a comment
There was a problem hiding this comment.
Still need to do a full review, however some initial questions/comments.
|
|
||
| exit: | ||
| if (instructions->signal > 0) { | ||
| signal(instructions->signal, SIG_DFL); |
There was a problem hiding this comment.
Do we also need something like
sigset_t set;
sigemptyset(&set);
sigaddset(&set, instructions->signal);
sigprocmask(SIG_UNBLOCK, &set, NULL);
to unblock the signal mask? As it's possible the fork will inherit the masked state with an already existing block and at this point the signal will not terminate correctly? The commit already hints at the fact it could be masked, so is there a reason we don't unblock it?
There was a problem hiding this comment.
This is getting into YAGNI territory, but I've made the signal masking more robust.
|
|
||
| switch (fault) { | ||
| case FORK_FAIL_FORK: | ||
| resource = RLIMIT_NPROC; |
There was a problem hiding this comment.
minor: If I've understood correctly this code begins a process which essentially sets RLIMIT_NPROC to 0, with the hope this will cause the fork to fail. Will this work for all cases as it can be bypassed for privileged users and can behave differently under container/runtime setups and do we care? If we do maybe a hook would be better?
There was a problem hiding this comment.
Resource limits are simple and relatively portable. (I've restricted them to Linux, but I think it's mostly a matter of coercing headers to declare the right things, but also possibly of some OSes not enforcing certain limits.)
Hooking into a system call is more dependent how you build the code and on the runtime environment. I'm not sure if it would work on WSL or Cygwin, for example, whereas resource limits should work (although I haven't verified that, and I don't care all that much if they don't work).
A container shouldn't matter (and can make hooking into system calls harder). Running those tests as a privileged user won't work, but that's not something we would do, so I don't think that matters, but I've added a comment.
… entry Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
2 participants
Together with its framework prerequisite, resolves Mbed-TLS/mbedtls-framework#295.
I am adding a test suite
test_suite_metatestwhose sole job is to test code that is in the test framework. Ideally, this test suite should be in the test framework. However we don't currently have a place for unit tests in the test framework, and I don't think it's worth the trouble to add them. So instead I am adding the tests in a consuming branch. They don't need to be backported unless the test code that's being tested starts having different behavior based on the consuming branch.Needs preceding PR: Mbed-TLS/mbedtls-framework#297
PR checklist