Page MenuHomeFreeBSD

Integrate capsicum-test into the FreeBSD test suite
ClosedPublic

Authored by ngie on Mar 29 2019, 10:39 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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ngie created this revision.Mar 29 2019, 10:39 PM
ngie edited the summary of this revision. (Show Details)Mar 29 2019, 10:40 PM
ngie edited the summary of this revision. (Show Details)
ngie edited the summary of this revision. (Show Details)Mar 29 2019, 10:44 PM
ngie edited the summary of this revision. (Show Details)Mar 29 2019, 10:47 PM
ngie added a subscriber: capsicum.
ngie edited the summary of this revision. (Show Details)Mar 29 2019, 10:52 PM

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.

ngie added a comment.Mar 30 2019, 12:08 AM

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 marked 3 inline comments as done.Mar 30 2019, 12:16 AM
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 edited the summary of this revision. (Show Details)Mar 31 2019, 4:50 AM
ngie added a subscriber: markj.
ngie edited the summary of this revision. (Show Details)Mar 31 2019, 5:07 AM
ngie updated this revision to Diff 55642.Mar 31 2019, 5:23 AM
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.
Harbormaster completed remote builds in B23414: Diff 55642.
ngie updated this revision to Diff 55643.Mar 31 2019, 5:24 AM

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

ngie edited the test plan for this revision. (Show Details)Mar 31 2019, 5:28 AM
ngie updated this revision to Diff 55652.Mar 31 2019, 4:59 PM

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?

ngie added a comment.Mar 31 2019, 6:16 PM

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.

ngie updated this revision to Diff 55658.Mar 31 2019, 11:23 PM

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

ngie edited the summary of this revision. (Show Details)Apr 1 2019, 6:33 PM
ngie updated this revision to Diff 55685.Apr 1 2019, 6:36 PM

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

markj added inline comments.Apr 1 2019, 7:33 PM
contrib/capsicum-test/capsicum-test-main.cc
58 ↗(On Diff #55685)

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.

emaste added inline comments.Apr 1 2019, 7:37 PM
contrib/capsicum-test/capsicum-test-main.cc
58 ↗(On Diff #55685)

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

ngie updated this revision to Diff 55696.Apr 1 2019, 8:07 PM
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.

ngie added inline comments.Apr 1 2019, 8:07 PM
contrib/capsicum-test/capsicum-test-main.cc
58 ↗(On Diff #55685)

You're both right; the message was misleading.

Updated!

asomers accepted this revision.Apr 1 2019, 8:16 PM

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*.

ngie edited the summary of this revision. (Show Details)Apr 1 2019, 8:25 PM

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.

markj added inline comments.Apr 1 2019, 8:47 PM
contrib/capsicum-test/capsicum-test-main.cc
60 ↗(On Diff #55696)

s/it's/its/

emaste added a comment.Apr 1 2019, 8:57 PM

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
ngie updated this revision to Diff 55700.Apr 1 2019, 9:11 PM

Fix grammar nit

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
emaste added a comment.Apr 1 2019, 9:15 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.