On 2022/11/13 13:34:52 +0100, Alexander Klimov <grandmas...@al2klimov.de> wrote:
>
>
> On 13.11.22 12:36, Stuart Henderson wrote:
> > One minor issue, argp-standalone only provides a static library, so
> > this should use BUILD_DEPENDS rather than LIB_DEPENDS (essentially
> > LIB_DEPENDS is invalid unless there is a WANTLIB entry to go with it).
+1
> > I would like guidance from someone who knows pledge better than me
> > about the NULL vs "" for execpromises. The manual doesn't mention it
> > but AFAIK we are not using it at all and I think it would be unwise
> Well...
> grep -rnIFe 'pledge("' /usr/ports |grep -vFwe NULL -e exec
> says:
can't speak for these
> /usr/ports/devel/woboq_codebrowser/patches/patch-generator_main_cpp:25:+
> if (pledge("stdio rpath wpath cpath tty", "") == -1) {
> /usr/ports/devel/woboq_codebrowser/patches/patch-indexgenerator_indexer_cpp:24:+
>
> if (pledge("stdio rpath wpath cpath tty", "") == -1) {
but these are like that because upstream decided to use "" for
execpromises. When I fixed the pledge promise there I kept upstream
style.
> /usr/ports/mail/mblaze/patches/patch-mdeliver_c:11:- xpledge("stdio
> rpath wpath cpath", "");
> /usr/ports/mail/mblaze/patches/patch-mdeliver_c:12:+ xpledge("stdio
> rpath wpath cpath fattr", "");
> /usr/ports/mail/mblaze/patches/patch-mdeliver_c:15:+ xpledge("stdio
> rpath wpath cpath", "");
> ...
>
> > to do something different here.
the second argument to pledge applies only to the child processes. I
haven't read f3 code, but assuming it works (not tested) since it
lacks pledge "proc" I guess it's practically the same if execpromise
is NULL or "". fwiw I prefer NULL tho.
I think it was already mentioned in this thread, but i wouldn't
unveil(NULL, NULL) since you call pledge next line without the
"unveil" promise. Also, I'd prefer if it used something more usual
for pledge instead of assert. In base, you'd find
if (pledge("...", NULL) == -1)
err(1, "pledge");
which is a better idiom that assert(!pledge("...", NULL)) IMHO. (not
counting that assert can be dearmed with -DNDEBUG.)
Regarding patches/patch-utils_c:
15 + if (chdir(*dev_path)) {
16 + err(errno, "Can't change working directory to %s at %s()
16 ", *dev_path, __func__);
17 + }
I don't know how useful it is to terminate the process with an exit
status of `errno'. I'd just err(1, "...");
Also, I'm not sure how I feel about trying to chroot and not reporting
the error if errno is EPERM. Probably better to check with geteuid()?
But still meh.
While pledge and unveil have some magic in the kernel so that some
kind of libc operations always works (reading the timezone data for
example) when using chroot you may need to initialize some things
explicitly. I haven't read the code, have you double checked? if in
doubt, I'd just drop the chroot patch from the port and send it
upstream. Other operating system could benefit from having this
chrooted, and upstream developer should have a clearer idea of when/if
to chroot.
The way the missing posix_fadvise is handled i guess it's fine.
Looking at the linux manpage it says it's to "announce an intention to
access file data in a specific pattern in the future, thus allowing
the kernel to perform appropriate optimizations" so neutering it
should be ok.
Just one more comment regarding the Makefile patch: I'd add a MANDIR
variable set to ${PREFIX}/share/man/ and overwrite it in the port'
makefile
MAKE_ENV += MANDIR=${PREFIX}/man/
this way such patch could be easily upstreamed and removed from the
port in a future update.
Cheers,
Omar Polo
