Page MenuHomeFreeBSD
Differential D8076

Capsicum-ize lam(1)
ClosedPublic
Actions

Authored by allanjude on Sep 29 2016, 3:21 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, May 9, 8:24 PM
Unknown Object (File)
Thu, May 9, 8:24 PM
Unknown Object (File)
Thu, May 9, 8:24 PM
Unknown Object (File)
Thu, May 9, 8:23 PM
Unknown Object (File)
Thu, May 9, 8:23 PM
Unknown Object (File)
Thu, May 9, 12:24 PM
Unknown Object (File)
Dec 20 2023, 12:42 AM
Unknown Object (File)
Nov 8 2023, 5:26 AM
View All 47 Files
Subscribers
imp
jonathan
mmel

Details

Reviewers
emaste
cem
jonathan
oshogbo
Group Reviewers
capsicum
Commits
rS313938: Capsicum-ize lam(1)
Summary

lam will be used in portsnap after D8074

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

allanjude updated this revision to Diff 20816.Sep 29 2016, 3:21 PM
allanjude retitled this revision from to Capsicum-ize lam(1).
allanjude updated this object.
allanjude edited the test plan for this revision. (Show Details)
allanjude added reviewers: capsicum, emaste, oshogbo, cem.
Herald added a subscriber: imp. · View Herald TranscriptSep 29 2016, 3:21 PM
allanjude updated this revision to Diff 20818.Sep 29 2016, 4:46 PM

Move the boilerplate code to a static function

This can be replaced with the capsicum helper function/library when it arrives

jonathan accepted this revision.Oct 3 2016, 12:48 AM
jonathan added a reviewer: jonathan.
jonathan added a subscriber: jonathan.

Looks good to me in principle; see inline comment above.

It seems like we now have enough experience confining std{in,out,err} to extract that into a library function?

usr.bin/lam/lam.c
92 ↗(On Diff #20818)

Could we use something from sysexits.h here (e.g., EX_OSERR) rather than the value 1?

This revision is now accepted and ready to land.Oct 3 2016, 12:48 AM
emaste added inline comments.Oct 3 2016, 12:58 AM
usr.bin/lam/lam.c
92 ↗(On Diff #20818)

According to bde@ and others sysexits is a mistake; see e.g. https://lists.freebsd.org/pipermail/svn-src-head/2013-January/044174.html. Style(9) just says Exits should be 0 on success, or 1 on failure.

cem added inline comments.Oct 3 2016, 3:50 PM
usr.bin/lam/lam.c
92 ↗(On Diff #20818)

I don't think BDE is right about that, but 1 is fine for simple programs that aren't already using sysexits-style codes.

emaste accepted this revision.Oct 3 2016, 3:52 PM
emaste edited edge metadata.
cem requested changes to this revision.Oct 3 2016, 4:46 PM
cem edited edge metadata.
cem added inline comments.
usr.bin/lam/lam.c
164–165 ↗(On Diff #20818)

getargs() is invoked after the sandbox is entered. It seems unlikely fopen() will work.

This revision now requires changes to proceed.Oct 3 2016, 4:46 PM
cem accepted this revision.Oct 3 2016, 4:47 PM
cem edited edge metadata.

Maybe move setup_capsicum below getargs() and move cap_enter() into it.

usr.bin/lam/lam.c
164–165 ↗(On Diff #20818)

Just kidding. I see cap_enter() happens after setup_capsicum, for some reason.

This revision is now accepted and ready to land.Oct 3 2016, 4:47 PM
emaste added inline comments.Oct 3 2016, 4:51 PM
usr.bin/lam/lam.c
164–165 ↗(On Diff #20818)

I asked for something like setup_capsicum as a placeholder for the upcoming Capsicum helpers, but it seems the name can lead the reader astray. What if we just copy the necessary proposed Capsicum helpers into here for now, prior to them being committed?

cem added inline comments.Oct 3 2016, 4:56 PM
usr.bin/lam/lam.c
164–165 ↗(On Diff #20818)

I don't think we make our reviewee dance too much just to capsicum this program. It's fine as-is and either way, will require only one fixup change later after the helpers land.

allanjude updated this revision to Diff 21335.Oct 13 2016, 1:56 AM
allanjude edited edge metadata.

Update to use the new capsicum_helper.h

This revision now requires review to proceed.Oct 13 2016, 1:56 AM
cem accepted this revision.Oct 13 2016, 2:08 AM
cem edited edge metadata.
This revision is now accepted and ready to land.Oct 13 2016, 2:08 AM
emaste accepted this revision.Oct 20 2016, 9:07 PM
emaste edited edge metadata.
emaste added inline comments.
usr.bin/lam/lam.c
89 ↗(On Diff #21335)

extraneous blank line?

128 ↗(On Diff #21335)

seems like an extraneous blank line

cem added inline comments.Oct 20 2016, 9:35 PM
usr.bin/lam/lam.c
89 ↗(On Diff #21335)

For clarity.

128 ↗(On Diff #21335)

Ok.

cem added a comment.Oct 20 2016, 9:52 PM

Whoops! Not my review. Sorry :-).

emaste added a comment.Dec 21 2016, 11:52 PM

Ping?

Closed by commit rS313938: Capsicum-ize lam(1) (authored by allanjude). · Explain WhyFeb 19 2017, 6:03 AM
This revision was automatically updated to reflect the committed changes.
allanjude marked 4 inline comments as done.
mmel added a subscriber: mmel.Mar 13 2017, 5:03 PM

It seems that this breaks lam (and thus portsnap) on systems without capsicum. Many tier2 systems are affected by this.

root@tegra124:/usr/ports # portsnap fetch
Looking up portsnap.FreeBSD.org mirrors... 6 mirrors found.
Fetching snapshot tag from ec2-eu-west-1.portsnap.freebsd.org... done.
Fetching snapshot metadata... done.
Updating from Sat Jan 28 07:27:13 UTC 2017 to Mon Mar 13 16:02:31 UTC 2017.
Fetching 5 metadata patches.lam: unable to limit rights on: -: Function not implemented
done.
Applying metadata patches... done.
Fetching 5 metadata files... lam: unable to limit rights on: -: Function not implemented
/usr/sbin/portsnap: cannot open a5dec4addaeb7dfd733a5eb227f3cdbb7f1f4ab2bd63426d5fd6e5d4e5be2277.gz: No such file or directory

metadata is corrupt.

cem added inline comments.Mar 13 2017, 5:14 PM
head/usr.bin/lam/lam.c
137

Needs && errno != ENOSYS, I guess.

Revision Contents
Changeset List

PathSize
head/
usr.bin/
lam/
20 lines

Diff 25373

View Options

head/usr.bin/lam/lam.c

Loading...