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

lib/libcasper/services/cap_fileargs/cap_fileargs.c
158–159

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

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?

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.

@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
47

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
9

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
2

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

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 added inline comments.
lib/libcasper/services/cap_fileargs/cap_fileargs.h
47

Not sure what kind of comment you thinking about?

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

do we have any measurement on the performance penalty?

Fix compilation without Casper.

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
101

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
47

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?

lib/libcasper/services/Makefile
12
lib/libcasper/services/cap_fileargs/cap_fileargs.3
101

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
47

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.