seccomp: fix large diff snapshot failure #2819
Conversation
Signed-off-by: alindima <alindima@amazon.com>
alindima
added
the
Status: Awaiting review
| } | ||
| ] | ||
| }, | ||
| { |
There was a problem hiding this comment.
Very good catch. However, I'm worried that we had to rely on a person finding this rather than our test. Did we just not cover this in any kind of test on aarch64? Maybe we can open an issue about this, might even be a "Good First Contribution" 😄 .
There was a problem hiding this comment.
We have a regression test for this in test_api.py::test_map_private_seccomp_regression, but for some reason this mmap call must not have been triggered on ARM.
It's very likely a platform-specific implementation detail of either the Global Allocator or the rust stdlib.
I'll try to see if sending a larger payload than the current 1 mib would trigger the regression on ARM
There was a problem hiding this comment.
Turns out that the regression test in test_api.py is not failing even if I remove the mmap from the allowlist.
Looking with strace, there are no such mmap calls issued.
I think it's a bug in the test, I'm looking into it
There was a problem hiding this comment.
Fixed the bug in the test, it now tests the regression 👍🏻
Personally, I don't think it's worth adding a changelog entry |
| "val": 34, | ||
| "comment": " libc::MAP_ANONYMOUS | libc::MAP_PRIVATE" | ||
| "val": 1, | ||
| "comment": "libc::MAP_SHARED" |
There was a problem hiding this comment.
is the comment correct? MAP_SHARED being needed for reading the timezone?
There was a problem hiding this comment.
yes it is. I've checked with strace and in the musl implementation of localtime_r
It first checks for the TZ environment variable. If that's not available and if the /etc/localtime file is present, it opens the file and mmaps it.
During this commit: a3f5d47, the mmap call was mistakenly added to the vmm thread, as opposed to the API thread to which it was intended. Also fix the regression test. There was a bug in it, which lead to this issue being hidden. Signed-off-by: alindima <alindima@amazon.com>
Also adds regression test. Signed-off-by: alindima <alindima@amazon.com>
Reason for This PR
#2811
Description of Changes
seccomp: allow mmap(MAP_PRIVATE|MAP_ANONYMOUS) on the vmm thread.
add regression test for it
fix error from commit a3f5d47. The mmap call on aarch64 should have been allowed on the api thread, but was instead added to the vmm
This functionality can be added in
rust-vmm.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.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).unsafecode is properly documented.CHANGELOG.md.