fix seccomp unit tests output #1254
Conversation
serban300
force-pushed
the
fix-seccomp-ut
branch
from
7a7da84
to
eb12ba2
Compare
serban300
force-pushed
the
fix-seccomp-ut
branch
2 times, most recently
from
5bb919c
to
1c53003
Compare
seccomp/src/lib.rs
Outdated
| // apply filter | ||
| // We need to execute the seccomp thread inside a catch_unwind block in order to | ||
| // avoid printing unneeded warnings in case of a seccomp denial. | ||
| let seccomp_result = panic::catch_unwind(|| { |
There was a problem hiding this comment.
The problem with this approach is that we won't be able to get the error message from asserts. It also seems we are testing more than one thing in this helper function.
I find it a bit shaky to test that SIGSYS was triggered using a boolean. We can't know for sure if the boolean value was not updated because the thread was killed for another reason or because of SIGSYS.
I don't really have another approach of testing it right now. We should check if having separate tests for negative testing is feasible and if in these tests we can count on should_panic macro.
There was a problem hiding this comment.
I agree that the logic is a bit shaky since there is no way to identify a SIGSYS for sure. But should_panic won't change this. Testing this with unit tests is a bit of a stretch. Maybe we should remove the unit tests that rely on this logic.
There was a problem hiding this comment.
I suppose the unit test was added as a regression test. I am fine with not testing this with unit tests as long as we have an integration test that checks for regressions.
There was a problem hiding this comment.
I don't think we can do this either. We would need to start firecracker with some very specific seccomp rules and these are not configurable from the outside.
There was a problem hiding this comment.
Could this be plugged into the existing seccomp integration test in integration_tests/security?
There was a problem hiding this comment.
While this test is not 100% conclusive (it can, in some cases, provide false negatives) it's still better than nothing.
Unless we find a better solution I say we keep the test and maybe add a comment specifying why this test doesn't fully guarantee SIGSYS correctness.
Better to get some validation than no validation 😄
There was a problem hiding this comment.
Looks like we aren't the only humans having problem with these kinds of tests: rust-lang/rust#32512
It doesn't look like it's fixed just yet.
There was a problem hiding this comment.
The logic gets much cleaner if we use SECCOMP_RET_ERRNO . Credits to @alexandruag for the idea.
@andreeaflorescu @acatangiu @sandreim
Please take another look on the changes.
There was a problem hiding this comment.
I'm learning seccomp with these reviews 😄
serban300
force-pushed
the
fix-seccomp-ut
branch
from
1c53003
to
5529f1b
Compare
We have some seccomp unit tests that rely on SECCOMP_RET_KILL. By changing them to use SECCOMP_RET_ERRNO instead we make them simpler and more reliable. Signed-off-by: Serban Iorga <[email protected]>
serban300
force-pushed
the
fix-seccomp-ut
branch
from
5529f1b
to
aae146b
Compare
Signed-off-by: Serban Iorga [email protected]
Reason for This PR
Fixes #1247
Description of Changes
Fix #1247
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.
PR Checklist
[Author TODO: Meet these criteria. Where there are two options, keep one.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).