@kib we should probably #define O_DSYNC O_SYNC in sys/ headers?

Discussed with @oshogbo, in my opinion it's a bug that we need to -DWITH_CASPER to use our headers -- but an issue to be addressed outside of this review.

Having -O0 -g hardcoded is wrong (very wrong). And why not use normal freebsd build ?

Formally defining O_DSYNC as O_SYNC is correct according to the POSIX requirements, but it is wrong on its intent. This is why we did not provided fake fdatasync(2) and I implemented real syscall for UFS.

On the other hand, #define O_RSYNC 0 is correct because we use rangelocks and never allow reader to see partial write.

We'd certainly do that if importing into base; this is a patch against upstream's repo. Main goal in this phab review is to present the proposed approach to Capsicum use.

This change is wrong on e.g., Linux.

It would probably make things easier if we just shipped a compatibility <endian.h> in FreeBSD that included sys/endian.h.

Should this stuff be #ifdef __FreeBSD__?

What's the purpose of this change? It introduces a couple major behavioral differences:

1. On error, caller's *fl is freed; it was not before.
2. memcpy will be a NULL pointer dereference in the error case, where it was not before.

So, open + fstat and lstat are slightly different, right? Chiefly, lstat doesn't follow symlinks, but open() does. AFAIK we can't actually open symlinks as pseudo-files, even with openat + O_NOFOLLOW. So we're breaking the S_ISLNK() handling here (and in other places).

It seems what we need is a fileargs_stat() (with flags), or _lstat().

Rather than adding close(fd); to every early return path after this, I'd suggest breaking the if/else chain:

if ((fd = fileargs...) < 0) {
    ...
    return 0;
}
rc = fstat(fd, ...);
close(fd);
if (rc == -1) {
    ...
} else if ( ... ) {

same issue with lstat != open+fstat here

Same issue with fd leakage here.

I don't understand why we replaced the fork + waitpid mechanism with pdfork + kevent here (this is two questions — pdfork doesn't necessitate kqueue).

Seems like we could enter sandbox slightly earlier with CAP_CONNECT on the socket? May not matter too much.

Yes, I intentionally asked Bora to not worry about compatibility with other systems initially, just to understand the scope of the changes.

Is our sys/endian.h (mostly) equivalent to linux's endian.h?

See earlier comment - we'll need to work with Kristaps to determine what sort of portability goo is appropriate upstream.

For this one we either want to add recallocarray to libc, or just provide it in an openbsd compat lib or local file.

Yep, mostly included for the same macros/functions (be32toh, etc). Linux's endian.h lacks our *dec() and *enc() routines.

We already have reallocarray in libc

Right, this one is recallocarray - or perhaps you're pointing out the precedent of importing these sort of functions into libc?

waitpid is not allowed once you cap_enter. The only other way to waitpid + get the exit code is kevent that I found and I need the process descriptor for that.

Doh, I missed that c both times. Mea culpa. Yeah, +1 to importing it or otherwise implementing a shim, instead of this.

I'm confused where we cap_enter. Is it during rsync_client()?

I'm also confused why we don't allow waitpid in capability mode. Maybe historical artifact? Obviously, we expose the same information via kevent, so either it should be allowed, or kevent shouldn't be...

rsync_client() calls either rsync_sender() or rsync_receiver(). That's where we cap_enter. So technically yes, we're in capability mode after rsync_client.

This is the only reference I found as to why waitpid isn't allowed https://lists.cam.ac.uk/pipermail/cl-capsicum-discuss/2010-November/msg00001.html

There is also supposed to be a pdwait but that's not implemented.

This email from Robert on that thread seems to suggest waitpid should just be allowed: https://lists.cam.ac.uk/pipermail/cl-capsicum-discuss/2010-November/msg00003.html

as earlier, caph_enter

also we should leave as ERR() not ERRX(), the errno is valid and useful information

For reference: @markj has been looking at endian.h

That was a draft comment from years ago that I accidentally submitted just now. @imp added include/endian.h in 30e0d2a51026830e5141ce3dd43854819bba910c

I think @imp ended up doing that work, see commit 30e0d2a510268.