Page MenuHomeFreeBSD

Introduce cap_fileargs.
ClosedPublic

Authored by oshogbo on Feb 17 2018, 12:27 PM.

Details

Summary

Casper service for handle files passed over argv/argc.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

oshogbo created this revision.Feb 17 2018, 12:27 PM
emaste added inline comments.Feb 17 2018, 12:59 PM
lib/libcasper/services/cap_fileargs/cap_fileargs.c
159–160

I wonder if it makes sense to allow empty args as a trivial case, which could slightly simplify cap_fileargs use?

oshogbo updated this revision to Diff 39588.Feb 21 2018, 9:58 PM

We handle now empty argc and argv as @emaste suggested.

I think this looks good. (Note that I haven't yet reviewed in detail.)

I wonder how hard it would be to extend / provide an alternate API allowing per-file flags/modes? Allowing us to support something like pdftk with a commandline such as pdftk in1.pdf in2.pdf cat output out1.pdf. It would require application-specific code to parse all arguments in advance, but would be quite convenient to create a fileargs context with a readable in1.pdf and in2.pdf and writable out1.pdf.

I noticed that we're missing manpages for the casper services and submitted PR 226102 to track that; we should add a manpage for cap_fileargs too. I can try to help create it.

So for know the idea is to just create two or three whatever fileargs instance as you need with difference rights.
If we want that in one service the question is do we need it in fileargs or in filesystem one?

See D14408 and D14409 for example use case

So for know the idea is to just create two or three whatever fileargs instance as you need with difference rights.

Ok. IMO then we should keep this as is, and just note in the man page that the API may be changed or modified in the future.

emaste added a subscriber: kevans.Feb 26 2018, 2:43 AM

@cem, @kevans: I think hexdump is probably a good candidate for the next program to apply fileargs, as we've currently implemented enter-capability-mode-on-last-argument.

I've applied this patch (and head, wc) to my WIP tree for test and further review.

lib/libcasper/services/cap_fileargs/cap_fileargs.h
48

Perhaps the stubs deserve a comment?

Other than a couple of small comments I think this is good to go.

lib/libcasper/services/cap_fileargs/Makefile
10

Consider using 1 as with other casper services? (I think users may see a lone .so.0 and assume it is outdated.)

lib/libcasper/services/cap_fileargs/cap_fileargs.h
3

Ought to add a SPDX-License-Identifier: BSD-2-Clause-FreeBSD (and to the .c)

oshogbo updated this revision to Diff 39765.Feb 26 2018, 7:06 PM
oshogbo marked an inline comment as done.

Fortuneteller I'm very poor in writing man pages but here my try!

I also updated most of the request from @emaste .
And fix one mock.

oshogbo marked 2 inline comments as done.Feb 26 2018, 7:07 PM
oshogbo added inline comments.
lib/libcasper/services/cap_fileargs/cap_fileargs.h
48

Not sure what kind of comment you thinking about?

emaste added inline comments.Feb 27 2018, 9:18 PM
lib/libcasper/services/cap_fileargs/cap_fileargs.h
84
In file included from /scratch/tmp/emaste/freebsd/usr.bin/head/head.c:60:
/scratch/tmp/emaste/obj/scratch/tmp/emaste/freebsd/amd64.amd64/tmp/usr/include/casper/cap_fileargs.h:82:6: error: implicit declaration of function 'dnvlist_get_number' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
            dnvlist_get_number(limits, "mode", 0));
            ^

With a trivial patch to cap_fileargs.h to fix the build and with D14409 applied, head fails like so:

% make WITHOUT_CASPER=yes -C usr.bin/head
...
% usr.bin/head/obj/head /tmp/foo
head: /tmp/foo: Not permitted in capability mode
bapt added a comment.Mar 21 2018, 5:04 PM

do we have any measurement on the performance penalty?

araujo added a subscriber: araujo.Apr 4 2018, 5:39 AM
oshogbo updated this revision to Diff 43157.May 30 2018, 6:34 PM

Fix compilation without Casper.

jonathan added inline comments.Jun 6 2018, 10:47 PM
lib/libcasper/services/Makefile
12

I don't see a tests directory in the review... am I just missing it?

lib/libcasper/services/cap_fileargs/cap_fileargs.3
102

Rather than providing two separate functions, might it make sense to have one fileargs_init function that takes an optionally-NULL argument?

lib/libcasper/services/cap_fileargs/cap_fileargs.h
48

It feels a bit odd to me that the user of this interface is required to know the details of the implementation, e.g., whether fileargs or an nvlist are used to store the descriptors. Couldn't this be hidden behind an opaque type with the nv-or-plain-structure choice as an implementation detail?

oshogbo added inline comments.Jun 9 2018, 5:36 PM
lib/libcasper/services/Makefile
12
lib/libcasper/services/cap_fileargs/cap_fileargs.3
102

I open for such change.
When I was creating this I was thinking that this will allow us to mitigate error when the creation of casper process will fail and somebody will pass NULL by mistake to this function.

But I don't have strong opinion about that.

lib/libcasper/services/cap_fileargs/cap_fileargs.h
48

This is optional function only for optimization purpose. Normally you would use fileargs_init, fileargs_initnv I found usefully when I need to pares somehow argc/argv and I don't want to create extra buffer and copy memory etc... I left all that operation to be handled by nvlist.

Any news about this patch? I need it to be able to finish this review: https://reviews.freebsd.org/D12419

Best,

This revision was not accepted when it landed; it landed in state Needs Review.Nov 12 2018, 5:41 PM
This revision was automatically updated to reflect the committed changes.