Page MenuHomeFreeBSD

Integrate capsicum-test into the FreeBSD test suite
ClosedPublic

Authored by ngie on Mar 29 2019, 10:39 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Mar 10, 1:46 PM
Unknown Object (File)
Sun, Mar 10, 1:42 PM
Unknown Object (File)
Sun, Mar 10, 1:41 PM
Unknown Object (File)
Sun, Mar 10, 1:41 PM
Unknown Object (File)
Sun, Mar 10, 1:41 PM
Unknown Object (File)
Sun, Mar 10, 1:41 PM
Unknown Object (File)
Sun, Mar 10, 1:41 PM
Unknown Object (File)
Sun, Mar 10, 1:41 PM

Details

Summary

This change takes capsicum-test from upstream and applies some local changes to make the
tests work on FreeBSD when executed via Kyua.

The local modifications are as follows:

  1. Make OpenatTest.WithFlag pass with the new dot-dot lookup behavior in FreeBSD 12.x+.
  2. capsicum-test references a set of helper binaries: mini-me, mini-me.noexec, and mini-me.setuid, as part of the execve/fexecve tests, via execve, fexecve, and open. It achieves this upstream by assuming mini-me* is in the current directory, however, in order for Kyua to execute capsicum-test, it needs to provide a full path to mini-me*. In order to achieve this, I made capsicum-test cache the executable's path from argv[0] in main(..) and use the cached value to compute the path to mini-me* as part of the execve/fexecve testcases.
  3. The capsicum-test test suite assumes that it's always being run on CAPABILITIES enabled kernels. However, there's a chance that the test will be run on a host without a CAPABILITIES enabled kernel, so we must check for the support before running the tests. The way to achieve this is to add the relevant feature_present("security_capabilities") check to SetupEnvironment::SetUp() and skip the tests when the support is not available. While here, add a check for kern.trap_enotcap being enabled. As noted by markj@ in https://github.com/google/capsicum-test/issues/23, this sysctl being enabled can trigger non-deterministic failures. Therefore, the tests should be skipped if this sysctl is enabled.

All local changes have been submitted to the capsicum-test project
(https://github.com/google/capsicum-test) and are in various stages of review.
Please see the following pull requests for more details:

  1. https://github.com/google/capsicum-test/pull/35
  2. https://github.com/google/capsicum-test/pull/41
  3. https://github.com/google/capsicum-test/pull/42

MFC after: 1 month

Test Plan
  • Ran the tests successfully on a CAPABILITIES-enabled 13.0-CURRENT kernel:
$ kyua test -k /usr/tests/sys/capsicum/Kyuafile 
bindat_connectat:bindat_connectat_1  ->  passed  [0.004s]
bindat_connectat:bindat_connectat_2  ->  passed  [0.002s]
bindat_connectat:bindat_connectat_3  ->  passed  [0.004s]
capsicum-test:main  ->  passed  [37.367s]
ioctls_test:cap_ioctls__listen_copy  ->  passed  [0.006s]

Results file id is usr_tests_sys_capsicum.20190331-052742-581531
Results saved to /home/ngie/.kyua/store/results.usr_tests_sys_capsicum.20190331-052742-581531.db

5/5 passed (0 failed)
  • make tinderbox

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 23453
Build 22466: arc lint + arc unit

Event Timeline

ngie edited the summary of this revision. (Show Details)
ngie added a subscriber: capsicum.

So exactly which of those PRs are included? The commit message should make it very clear. Or better yet, there should be one commit to copy from vendor/, and another to apply local changes. Also, I think you should add the capabilities check before committing.

contrib/capsicum-test/syscalls.h
246 ↗(On Diff #55610)

Wow! I'm glad we don't have that problem.

contrib/capsicum-test/sysctl.cc
8 ↗(On Diff #55610)

This comment isn't accurate, but I suspect that it's an upstream problem.

etc/mtree/BSD.tests.dist
465 ↗(On Diff #55610)

Almost everything else in tests/sys is organized by its functional domain, not by its origin. It would be more consistent it capsicum-test were a subdirectory of capsicum, or even if the two directories were merged together.

tests/sys/capsicum-test/Makefile
28 ↗(On Diff #55610)

Shouldn't smoketest be added to GTESTS or PROGS ? And regardless, there's no reason to split lines when there's only a single source file.

So exactly which of those PRs are included? The commit message should make it very clear. Or better yet, there should be one commit to copy from vendor/, and another to apply local changes. Also, I think you should add the capabilities check before committing.

I should note that this is a very rough draft review.

Your points are absolutely correct: the proposed commit message is not at all clear. I'll fix it up in a few.

ngie added inline comments.
contrib/capsicum-test/syscalls.h
246 ↗(On Diff #55610)

Yup. Good ole Linux.

contrib/capsicum-test/sysctl.cc
8 ↗(On Diff #55610)

Correct.

etc/mtree/BSD.tests.dist
465 ↗(On Diff #55610)

Now I remember the method to my madness (!).

I wanted to avoid compiling capsicum-test if MK_GOOGLETEST == no, and avoid complexity in tests/sys/capsicum/Makefile. I also ran into some fun issues with WARNS, but I suspect (now), that that's due to the fix in D19755 not being in.

Unfortunately, it wouldn't work as a subdir, unless it installed to a different directory: the Kyuafile generated from the capsicum directory would conflict with the capsicum-test directory.

tests/sys/capsicum-test/Makefile
28 ↗(On Diff #55610)

I removed it, since it was an unnecessary auxiliary program.

ngie retitled this revision from Integrate capsicum-test into the FreeBSD test suite to [DRAFT] Integrate capsicum-test into the FreeBSD test suite.Mar 30 2019, 12:17 AM
ngie edited the summary of this revision. (Show Details)
ngie added a subscriber: markj.
ngie marked 3 inline comments as done.
  1. Fold tests/sys/capsicum-test into tests/sys/capsicum, per request by @asomers.
  2. Add capsicum feature checks to SetupEnvironment::SetUp() to ensure capsicum is enabled and kern.trap_enotcap has not been enabled in the kernel.

Revert changes accidentally added in last update, which will be merged as part
of D19765.

Remove XXX comment/fold std::cerr into GTEST_SKIP()

It seems that D19765 is not functional as-is on the branch. Other commits are
missing from googletest master that would make this work.

ngie retitled this revision from [DRAFT] Integrate capsicum-test into the FreeBSD test suite to Integrate capsicum-test into the FreeBSD test suite.Mar 31 2019, 4:59 PM

Can you generate a review that's just the local diffs, i.e., after the svn cp?

Can you generate a review that's just the local diffs, i.e., after the svn cp?

Sure. I'll do this later on today.

Generate partial diff with just local modifications to contrib/capsicum-test and Makefile modifications

Add missing trailing period in skip message for kern.trap_enotcap.

contrib/capsicum-test/capsicum-test-main.cc
58

The problem isn't non-determinism, it's just that trap_enotcap causes the process to receive SIGTRAP when a syscall would otherwise return ENOTCAPABLE. There are lots of tests here that explicitly try to trigger that error.

contrib/capsicum-test/capsicum-test-main.cc
58

Most correct to state that the tests are skipped due to non-default setting of debugging sysctl, I suspect.

ngie marked 2 inline comments as done.

Fix kern.trap_enotcap message

@markj's right: determinism is not the problem; it being set invalidating test
results, is by sending SIGTRAP to processes that receive ENOTCAPABLE.

contrib/capsicum-test/capsicum-test-main.cc
58

You're both right; the message was misleading.

Updated!

LGTM. Just make sure that your commit message includes links to the upstream PRs that you're integrating.

tests/sys/capsicum-test/Makefile
28 ↗(On Diff #55610)

If you removed it, then why do you still set its SRCS ?

This revision is now accepted and ready to land.Apr 1 2019, 8:16 PM
ngie marked an inline comment as done.Apr 1 2019, 8:18 PM
ngie added inline comments.
tests/sys/capsicum-test/Makefile
28 ↗(On Diff #55610)

My adding it was because I was trying to directly port it from the capsicum-test project. Once I determined that it was no longer needed, I thought it was better to just drop the smoke test *shrugs*.

Ignore my last comment; it's obsolete because you _did_ remove SRCS.smoketest. It seems that Phabricator has changed. Now, when I click on an inline comment's line number link, it opens a new tab showing the code as it was at the time that I made the comment. That's super-annoying. The old behavior was better, but I'll try to get used to the new behavior.

contrib/capsicum-test/capsicum-test-main.cc
60

s/it's/its/

I think it makes sense to svn cp first, then apply the changes to the contrib/ location; is that your plan?

ngie marked an inline comment as done.EditedApr 1 2019, 9:08 PM

I think it makes sense to svn cp first, then apply the changes to the contrib/ location; is that your plan?

Yup. Here's how I've been updating the code for reviews (I broke the merge history somehow when I didn't properly reintegrate the googletest branch, so I'm resorting to manual patching):

$ cat capsicum-test-merge.sh 
#!/bin/sh
svn revert -R .
svn up
rm -Rf contrib/capsicum-test
svn copy ^/vendor/google/capsicum-test/dist contrib/capsicum-test
svn diff . ^/projects/capsicum-test/ > patch
vim patch
patch -V none < patch
This revision now requires review to proceed.Apr 1 2019, 9:11 PM
ngie marked an inline comment as done.Apr 1 2019, 9:13 PM
In D19758#424232, @ngie wrote:

I think it makes sense to svn cp first, then apply the changes to the contrib/ location; is that your plan?

Yup. Here's how I've been updating the code for reviews (I broke the merge history somehow when I didn't properly reintegrate the googletest branch, so I'm resorting to manual patching):

svn copy ^/vendor/google/capsicum-test/dist contrib/capsicum-test

OK - what I meant was commit up to the svn copy separately - in fact, go ahead with that at any time.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 1 2019, 9:25 PM
This revision was automatically updated to reflect the committed changes.