I wrote a regression test case for this bug, will commit it after this patch lands.

Some test cases were added to usr.bin/truss/tests recently. I wonder if they pass when truss is running in capmode?

Why is capability mode optional?

Looks good to me, just a small comment.

In D59309#1360831, @kib wrote:

You could memset only device_name then, right before strcpy(). It would be more clear IMHO.

A heretofore undiscovered (and unmitigated) exploit that allows an attacker to write to a region of memory may allow setting one but not the other.

In D59309#1360722, @olce wrote:

Use of memset() should be systematic on stack-allocated structures for security reasons. The compiler normally eliminates the dead stores.

In D59307#1360734, @bapt wrote:

*.* |exec /home/bapt/crap.sh

cat /home/bapt/crap.sh

#!/bin/sh
id
echo plop
sleep 1

It creates defunct for me on latest current without my patch.

I can't reproduce any zombie leak if I do something like *.* |sleep 10 in syslog.conf. Do you have some repro steps?

In D59309#1360722, @olce wrote:

Use of memset() should be systematic on stack-allocated structures for security reasons. The compiler normally eliminates the dead stores.

That said, here, it's unclear to me there is a problem in the first place: All fields are effectively filled, they are all 4-byte aligned, including the fields with enumeration type (the underlying type being int), and the total structure size is a multiple of 8, so there's no padding anywhere. Which are the bytes that are actually leaked?

In D59130#1360708, @dteske wrote:

Context.

All my seceng colleagues point out that unquoted parameter expansion with the unintended consequences of glob expansion is a lack of defense-in-depth architecture with security implications.

In that context, this is very much a security bug.

In D59130#1360660, @dteske wrote:

In D59130#1360640, @markj wrote:

In D59130#1360422, @dteske wrote:

In D59130#1356525, @markj wrote:

What security problem is being fixed here?

Denial-of-Service.

A poisoned LOCALBASE value such as:

LOCALBASE='/*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/*'

NB: That's 5x (10x /*)

Causes massive disk I/O and only in the minority of cases with little-to-no filesystem contents does it only result in a minor inconvenience (sub 30 second delay in launch) whereas on moderately to substantially loaded filesystems (with many files) does it in-turn result in never getting passed the : ${LOCALBASE:=...} line.

What's going on when you don't quote the parameter expansion, is that globs get expanded. Yes, it's true that : throws those expanded arguments away, but the damage is done in the I/O and compute required to expand those globs.

Quoting the parameter expansion (as-in : "${LOCALBASE:=...}" prevents any globs stuffed into a poisoned LOCALBASE rendered inert.

Literally ShellCheck's SC2223 DoS-via-glob warning.

If someone can modify your environment variables, you're in a bad place already, e.g., if they point ARC_CMD to a script of their choosing.

While imaginative (injecting a 3rd party into the mix via "someone") that's not exactly the concern.

In D59084#1359548, @christos wrote:

In D59084#1358453, @markj wrote:

The patch looks like it does what the review description says it does, but apparently it doesn't actually fix the cited PR?

Yes. I cited the PR because it does fix an issue raised indirectly in the PR. Should I omit the "PR" line?

In D59130#1360422, @dteske wrote:

In D59130#1356525, @markj wrote:

What security problem is being fixed here?

Denial-of-Service.

A poisoned LOCALBASE value such as:

LOCALBASE='/*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/*'

NB: That's 5x (10x /*)

Causes massive disk I/O and only in the minority of cases with little-to-no filesystem contents does it only result in a minor inconvenience (sub 30 second delay in launch) whereas on moderately to substantially loaded filesystems (with many files) does it in-turn result in never getting passed the : ${LOCALBASE:=...} line.

What's going on when you don't quote the parameter expansion, is that globs get expanded. Yes, it's true that : throws those expanded arguments away, but the damage is done in the I/O and compute required to expand those globs.

Quoting the parameter expansion (as-in : "${LOCALBASE:=...}" prevents any globs stuffed into a poisoned LOCALBASE rendered inert.

Literally ShellCheck's SC2223 DoS-via-glob warning.

Why not use atomics?

In D59276#1360076, @kp wrote:

That's an interesting idea.

Does running bhyveload ... </dev/null not provide the desired behaviour?

More comments.

The patch looks like it does what the review description says it does, but apparently it doesn't actually fix the cited PR?

Improve messages

In D59016#1354772, @igoro wrote:

Probably a simple parameter like in the following patch could be good enough and a bit more descriptive: https://reviews.freebsd.org/D59067.

This is a bit more complex than I was hoping. I do wonder if we really need all of these separate callbacks, vs. one callback for the initial copyin plus the header and trailer copyins. That would require a bit more code in the callbacks but seems simpler overall.

In D59164#1356972, @dteske wrote:

I get what you're trying to do. However, I would suggest that instead of a separate sub-command which cannot stand on its own, that we try to leverage common knowledge.

If we emulate "git rebase" by implementing a "--continue" and "--abort" then when a patch fails to apply, we can tell the user that they should try and resolve conflicts and then execute one of:

git arc patch --continue
git arc patch --abort

The combination of (1) catching the error and (2) telling the user to use one of these two (3) saves the user from having to retype the diff argument, at (4) the simple cost of a small cache file that is cleaned up on success.

That would provide direct access to the final patch_commit stage without inventing a partial wrapper over the process as a new sub-command.

Also, --abort could restore the tree from before patch started.