Add fileargs_lstat to cap_fileargs service

[borako.ozarslan_gmail.com]

Summary

Add fileargs_lstat function to cap_fileargs casper service to be able to lstat files
while in capability mode. It can only lstat files given in fileargs_init

Submitted by: Bora Ozarslan [e-mail]

[emaste]

lib/libcasper/services/cap_fileargs/cap_fileargs.h

50

name instead of path for consistency with the other entries? although really pathname or path is probably preferable for all of them, and then headers shouldn't really have arg names in any case to avoid conflicts with what is really in the application namespace

[cem]

I did not look too closely at the caching bits. The rest looks mostly good. Some feedback below.

lib/libcasper/services/cap_fileargs/cap_fileargs.3

27

Bump this when it is committed

134

is equivalent to
.Xr lstat 2 .

(See similar examples below.)

lib/libcasper/services/cap_fileargs/cap_fileargs.c

88–89

Maybe assert size == sizeof(*sb)?

365

style nit: int goes on its own line; fileargs_lstat begins the next line.

397–398

same as above

405

does buf need to be freed?

441–448

Why not just return (lstat(name, sb));?

621–623

Errno information is useful too. Maybe it would be good to pass this back through the nvlist layer somehow. (Out of scope for this revision.)

lib/libcasper/services/cap_fileargs/cap_fileargs.h

50

+1 name for consistency.

Removing all arg names should come as a separate cleanup, if at all. It's a low priority IMO (name isn't wholly unreasonable).

Style(9) nit: the * on a pointer declaration goes adjacent to the symbol name, not the type. So, struct stat *sb.

[emaste]

lib/libcasper/services/cap_fileargs/cap_fileargs.3

232–233

.Xrs are sorted first by section number then alphabetically; lstat should come before open.

lib/libcasper/services/cap_fileargs/cap_fileargs.c

441–448

just call lstat in fileargs_add_lstat_cache and fileargs_command_lstat?

[cem]

lib/libcasper/services/cap_fileargs/cap_fileargs.c

441–448

Indeed.

[borako.ozarslan_gmail.com]

lib/libcasper/services/cap_fileargs/cap_fileargs.c

405

nvlist man page specifically tells you to free it if you're using nvlist_take_* but doesn't say anything for nvlist_get_*. The difference is that former moves (deletes it from the list) the value and returns it as 'void *' whereas latter just gives access to it and returns it as a 'const void *'. So it looks like nvlist_destroy should free it if it needs to be freed if we're using nvlist_get_*. In the cache case it doesn't need to be freed as it has to live within the cache anyways.

441–448

Started by copying 'open_file' and didn't realize there was no need for it afterwards.

621–623

I think casper already looks at the return values of the service functions and puts the return value as ("error", number) pair into the nvlist. This is checked for in 'fileargs_fetch' and is used to set errno for example.

[borako.ozarslan_gmail.com]

Assess review comments

[oshogbo]

lib/libcasper/services/cap_fileargs/cap_fileargs.c

366

If we still need a lstat for some reasony it wouldnt make more sens to use cache to get a fd and run fstat on it, and if the fd is not available call casper?

405

If you use get you do not need to free/close anything. If you use take you have to.

674

else if

lib/libcasper/services/cap_fileargs/cap_fileargs.h

42

We can't just include header?

50

Why we addiding this in the first place? This is introduce big RC. Can we just in code use fstat()?

We also need version for user which do not use Casper. Otherwise you would need to do distinction in the code which use it. See else for WITH_CASPER macro.

[cem]

lib/libcasper/services/cap_fileargs/cap_fileargs.c

366

We can't open() a symlink to fstat

674

after return in previous if, I am fine eliding else here

[emaste]

lib/libcasper/services/cap_fileargs/cap_fileargs.c

674

Agreed, this is fine with me

lib/libcasper/services/cap_fileargs/cap_fileargs.h

42

We could, but what would be the benefit/value? When we're just dealing with pointers to opaque (to us) structs this seems reasonable.

50

We need lstat to determine if the provided name is a symlink.

[oshogbo]

lib/libcasper/services/cap_fileargs/cap_fileargs.c

366

Right. Bora do you have any example of use it in real program? I would like to check few things with the cache. I thing this breakes cache. The problem which I see is that we will need to travel whole nvlist of the cache to get lstat date. The nvlist probably looks something like:

[fd] [fd] [fd] [fd] [lstat] [lstat] [lstat] [lstat]

This would kill the performence.
Maybe you need another cache only for lstat or combine fd with lstat.

lib/libcasper/services/cap_fileargs/cap_fileargs.h

42

Ok.

[emaste]

lib/libcasper/services/cap_fileargs/cap_fileargs.c

366

See D19407 (openrsync port) for the example use case.

[emaste]

lib/libcasper/services/cap_fileargs/cap_fileargs.c

366

@oshogbo can you confirm my understanding, that the implementation walks a list to extract the requested cache entry and the concern here is that the list is twice as long? Bora and I chatted now and think moving it to the fd is the simplest, straightforward approach.

[cem]

lib/libcasper/services/cap_fileargs/cap_fileargs.c

366

Could both caches be made key-associative rather than linked lists? (Perhaps as a subsequent optimization?)

[emaste]

lib/libcasper/services/cap_fileargs/cap_fileargs.c

366

I think this is nvlist internals, would be a separate project.

[oshogbo]

I’m not having access to phab right now so I’m answering throug the email.
I hope that’s fine.

W dniu wt., 26.03.2019 o 01:13 emaste (Ed Maste) <
[e-mail]> napisał(a):

emaste added inline comments.

INLINE COMMENTS

oshogbo wrote in cap_fileargs.c:368
Right. Bora do you have any example of use it in real program? I would

like to check few things with the cache. I thing this breakes cache. The
problem which I see is that we will need to travel whole nvlist of the
cache to get lstat date. The nvlist probably looks something like:

[fd] [fd] [fd] [fd] [lstat] [lstat] [lstat] [lstat]

This would kill the performence.
Maybe you need another cache only for lstat or combine fd with lstat.

@oshogbo can you confirm my understanding, that the implementation walks a
list to extract the requested cache entry and the concern here is that the
list is twice as long? Bora and I chatted now and think moving it to the fd
is the simplest, straightforward approach.

the problem isn’t the lenght. The problem is that to iterate for a first
element of lstat you have to iterate over all file descriptors to get it.
So if we have 100 files to open, you will get first descriptor after 1
iteration and the first lstat for 101 operation. The secend fd after 1
operations and the secend lstat for the 100 operation. The fileargs right
now is optimized for iterating in a consistent loop as using argv/argc is
the common pattern.

We can have separate caches. We can combine fds with the stat I’m fine with
both. We can try to also add some flags to fileargs saying which operation
will be permitted. As if I pas FA_OPEN then I can only open the files.
FA_LSTAT allows me also to lstat the file names. This would determine how
cache works.

CHANGES SINCE LAST ACTION

https://reviews.freebsd.org/D19548/new/

REVISION DETAIL

https://reviews.freebsd.org/D19548

EMAIL PREFERENCES

https://reviews.freebsd.org/settings/panel/emailpreferences/

To: borako.ozarslan_gmail.com, emaste, oshogbo
Cc: cem

[borako.ozarslan_gmail.com]

Implemented a version for WITH_CASPER not defined.
Added a flag variable for stating which operations are allowed (FA_OPEN and FA_LSTAT).
Cache holds both fd and stat now but it's only filled if the operation is allowed.
fileargs_open takes the cache whereas fileargs_lstat doesn't, it just copies so the cache is correct until someone opens it.

[oshogbo]

lib/libcasper/services/cap_fileargs/cap_fileargs.c

60

The allowed operations should be checked on casper side not on client side.
You can imagine that somebody would exploit your application and ask casper to open file in case when the application should only lstat files.

558

style(9)
(allowed_operations & FA_OPEN) != 0

567

style.

lib/libcasper/services/cap_fileargs/cap_fileargs.h

90

Using here nvlist_get may detect for some errors in systems without casper.

[borako.ozarslan_gmail.com]

Move the operation validation to service side

[borako.ozarslan_gmail.com]

lib/libcasper/services/cap_fileargs/cap_fileargs.h

90

Is this what you meant?

[oshogbo]

lib/libcasper/services/cap_fileargs/cap_fileargs.h

90

Yes.

[pstef]

head/lib/libcasper/services/cap_fileargs/cap_fileargs.c

292 ↗

(On Diff #56289)

This is set, but never used.

[pauamma_gundo.com]

head/lib/libcasper/services/cap_fileargs/cap_fileargs.3

106 ↗

(On Diff #56289)

If the intent is emphasis, shouldn't this use .Sy instead?

[emaste]

Note this was committed to the tree a couple of years ago.

[pauamma_gundo.com]

In D19548#783853, @emaste wrote:

Note this was committed to the tree a couple of years ago.

Point. I didn't look at the timestamps or the number after D in the notification email I got from the previous comment. But the unused assignment to a local variable and the use of .Nm instead of .Sy are still both here. I also spotted something that could use editing in DESCRIPTION. I will poke the manual page when I have a moment, but someone else will have to deal with the source change as I have no way to test it.

[pstef]

In D19548#783853, @emaste wrote:

Note this was committed to the tree a couple of years ago.

I know, but I used this review to signal the set-but-unused-variable problem directly to people involved.