Page MenuHomeFreeBSD

Add fileargs_lstat to cap_fileargs service
ClosedPublic

Authored by borako.ozarslan_gmail.com on Mar 11 2019, 8:07 PM.

Details

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 borako.ozarslan@gmail.com

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

emaste added a subscriber: cem.
emaste added inline comments.Mar 11 2019, 9:29 PM
lib/libcasper/services/cap_fileargs/cap_fileargs.h
50 ↗(On Diff #54941)

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 added a comment.Mar 11 2019, 10:59 PM

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 ↗(On Diff #54941)

Bump this when it is committed

134 ↗(On Diff #54941)
is equivalent to
.Xr lstat 2 .

(See similar examples below.)

lib/libcasper/services/cap_fileargs/cap_fileargs.c
88–89 ↗(On Diff #54941)

Maybe assert size == sizeof(*sb)?

365 ↗(On Diff #54941)

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

397–398 ↗(On Diff #54941)

same as above

405 ↗(On Diff #54941)

does buf need to be freed?

441–448 ↗(On Diff #54941)

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

621–623 ↗(On Diff #54941)

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 ↗(On Diff #54941)

+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 added inline comments.Mar 12 2019, 1:47 PM
lib/libcasper/services/cap_fileargs/cap_fileargs.3
232–233 ↗(On Diff #54941)

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

lib/libcasper/services/cap_fileargs/cap_fileargs.c
441–448 ↗(On Diff #54941)

just call lstat in fileargs_add_lstat_cache and fileargs_command_lstat?

cem added inline comments.Mar 12 2019, 2:52 PM
lib/libcasper/services/cap_fileargs/cap_fileargs.c
441–448 ↗(On Diff #54941)

Indeed.

borako.ozarslan_gmail.com marked 12 inline comments as done.Mar 12 2019, 7:06 PM
borako.ozarslan_gmail.com added inline comments.
lib/libcasper/services/cap_fileargs/cap_fileargs.c
405 ↗(On Diff #54941)

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 ↗(On Diff #54941)

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

621–623 ↗(On Diff #54941)

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 marked 2 inline comments as done.Mar 12 2019, 7:07 PM
borako.ozarslan_gmail.com updated this revision to Diff 54996.

Assess review comments

oshogbo requested changes to this revision.Mar 21 2019, 2:20 AM
oshogbo added inline comments.
lib/libcasper/services/cap_fileargs/cap_fileargs.c
405 ↗(On Diff #54941)

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

368 ↗(On Diff #54996)

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?

665 ↗(On Diff #54996)

else if

lib/libcasper/services/cap_fileargs/cap_fileargs.h
42 ↗(On Diff #54996)

We can't just include header?

50 ↗(On Diff #54941)

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.

This revision now requires changes to proceed.Mar 21 2019, 2:20 AM
cem added inline comments.Mar 21 2019, 1:56 PM
lib/libcasper/services/cap_fileargs/cap_fileargs.c
368 ↗(On Diff #54996)

We can't open() a symlink to fstat

665 ↗(On Diff #54996)

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

emaste added inline comments.Mar 21 2019, 4:13 PM
lib/libcasper/services/cap_fileargs/cap_fileargs.c
665 ↗(On Diff #54996)

Agreed, this is fine with me

lib/libcasper/services/cap_fileargs/cap_fileargs.h
42 ↗(On Diff #54996)

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 ↗(On Diff #54941)

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

oshogbo added inline comments.Mar 23 2019, 11:55 PM
lib/libcasper/services/cap_fileargs/cap_fileargs.c
368 ↗(On Diff #54996)

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 ↗(On Diff #54996)

Ok.

emaste added inline comments.Mar 24 2019, 2:21 AM
lib/libcasper/services/cap_fileargs/cap_fileargs.c
368 ↗(On Diff #54996)

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

emaste added inline comments.Mar 25 2019, 4:13 PM
lib/libcasper/services/cap_fileargs/cap_fileargs.c
368 ↗(On Diff #54996)

@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 added inline comments.Mar 25 2019, 5:24 PM
lib/libcasper/services/cap_fileargs/cap_fileargs.c
368 ↗(On Diff #54996)

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

emaste added inline comments.Mar 25 2019, 9:22 PM
lib/libcasper/services/cap_fileargs/cap_fileargs.c
368 ↗(On Diff #54996)

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

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) <
phabric-noreply@freebsd.org> 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

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 requested changes to this revision.Apr 2 2019, 4:49 PM
oshogbo added inline comments.
lib/libcasper/services/cap_fileargs/cap_fileargs.c
60 ↗(On Diff #55502)

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.

502 ↗(On Diff #55502)

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

511 ↗(On Diff #55502)

style.

lib/libcasper/services/cap_fileargs/cap_fileargs.h
93 ↗(On Diff #55502)

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

This revision now requires changes to proceed.Apr 2 2019, 4:49 PM

Move the operation validation to service side

borako.ozarslan_gmail.com marked 4 inline comments as done.Apr 15 2019, 9:25 PM
borako.ozarslan_gmail.com added inline comments.
lib/libcasper/services/cap_fileargs/cap_fileargs.h
93 ↗(On Diff #55502)

Is this what you meant?

oshogbo accepted this revision.Apr 16 2019, 4:16 AM
oshogbo added inline comments.
lib/libcasper/services/cap_fileargs/cap_fileargs.h
93 ↗(On Diff #55502)

Yes.

This revision is now accepted and ready to land.Apr 16 2019, 4:16 AM
This revision was automatically updated to reflect the committed changes.