Page MenuHomeFreeBSD

[jail] removal by jid doesn't trigger pre/post stop scripts
ClosedPublic

Authored by mizhka on Aug 19 2019, 8:12 PM.

Details

Summary

Hi,

The "jail -r" doesn't trigger pre/post stop scripts (and more) described in config file if jid is specified instead of name. Debugging showed that in case of "removal by jid", jail doesn't fetch "cfjail" for jail list. Instead, it's create fake structure without scripts from config file.

The patch changes this code path to lookup jail from list of jails loaded from config file.

Test Plan

This patch works at least for me.
Few basic tests are written for regression testing.

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

mizhka created this revision.Aug 19 2019, 8:12 PM
kevans added a subscriber: kevans.Aug 20 2019, 12:54 PM
kevans added inline comments.
usr.sbin/jail/state.c
75 ↗(On Diff #61013)

I don't think this part can work. dep_setup is called exactly once- if !docf, then we never setup jails_byname just below and this condition is trivially true.

I would consider, perhaps, reworking running_jail ever-so-slightly to take a struct cfjail **jail and populate *jail if jail != NULL -- and always returning the jid if it's running.

mizhka added inline comments.Aug 20 2019, 1:17 PM
usr.sbin/jail/state.c
75 ↗(On Diff #61013)
  1. running_jail returns running cfjail* even if there is no config file. So this part is supposed to work as before.
  2. now we return cfjail with name and jid. So it's easy to fetch jid from cfjail. in case of extra argument (I supposed) it will be strange to return same info twice: as jid and as cfjail. If such API looks good, I will add extra argument. I preferred to keep API as simple as possible.
kevans added inline comments.Aug 20 2019, 1:20 PM
usr.sbin/jail/state.c
75 ↗(On Diff #61013)

I suspect this leads to a segfault before we get to fake the cfjail *, though, with an uninitialized jails_byname

mizhka updated this revision to Diff 61042.Aug 20 2019, 1:53 PM

Fix SEGFAULT on uninitialized jails_byname.

mizhka marked an inline comment as done.Aug 20 2019, 1:54 PM
mizhka updated this revision to Diff 61117.Thu, Aug 22, 2:45 PM

Add basic jail tests:

  • create/stop/list/restart jails
  • create/list nested jail
  • execute commands prestart and prestop

Few tests are not passed on my machine (13-CURRENT r349278):

  • prestop on removal by jid (origin issue)
  • "jls -n" doesn't work inside jail if there is nested jail (potential new issue)

Feel free to comment.

mizhka updated this revision to Diff 61136.Thu, Aug 22, 8:10 PM

Fix regression found thanks to tests:

  • "jail -r jailname" returned error: jail not found.

Now tests are passed

mizhka edited the test plan for this revision. (Show Details)Thu, Aug 22, 8:12 PM
mizhka added a reviewer: kristof.
kevans added inline comments.Sat, Aug 24, 3:14 AM
etc/mtree/BSD.tests.dist
1071 ↗(On Diff #61136)

Sorry- this should sort after fstyp

usr.sbin/jail/state.c
49 ↗(On Diff #61136)

Explicit initialization to NULL shouldn't be required, since this is a global

usr.sbin/jail/tests/jail_basic_test.sh
1 ↗(On Diff #61136)

We generally omit the shebang for atf-sh tests -- I don't recall the exact reasoning, though the framework (kyua, perhaps? or perhaps the build framework) will DTRT.

mizhka updated this revision to Diff 61266.Sun, Aug 25, 8:59 PM

Thank you, @kevans! fixed.

mizhka marked 3 inline comments as done.Sun, Aug 25, 9:00 PM
kevans accepted this revision.Wed, Sep 4, 1:16 PM

I think this looks good to me now

This revision is now accepted and ready to land.Wed, Sep 4, 1:16 PM
ray accepted this revision.Wed, Sep 4, 8:45 PM
ray added a subscriber: ray.

Like it. Thanks!

jamie accepted this revision.Thu, Sep 5, 10:10 PM

Aside from a few trailing whitespaces, this all looks good to me.

This revision was automatically updated to reflect the committed changes.