Page MenuHomeFreeBSD

libc: gen: refactor execvPe() for readability
ClosedPublic

Authored by kevans on Wed, Jul 30, 2:09 AM.
Tags
None
Referenced Files
F125696264: D51629.diff
Mon, Aug 11, 12:55 AM
Unknown Object (File)
Thu, Aug 7, 11:08 PM
Unknown Object (File)
Thu, Aug 7, 9:59 PM
Unknown Object (File)
Sun, Aug 3, 4:30 AM
Unknown Object (File)
Fri, Aug 1, 3:16 PM
Unknown Object (File)
Fri, Aug 1, 11:25 AM
Unknown Object (File)
Fri, Aug 1, 10:46 AM
Unknown Object (File)
Thu, Jul 31, 9:22 PM
Subscribers

Details

Summary

The current incarnation of execvPe() is a bit messy, and it can be
rather difficult to reason about whether we're actually doing the right
thing with our errors. We have two cases in which we may enter the loop:

1.) We have a name that has no slashes in it, and we enter the loop

normally through our strsep() logic to process $PATH

2.) We have a name with at least one slash, in which case we jump into

the middle of the loop then bail after precisely the one iteration
if we failed

Both paths will exit the loop if we failed, either via jumping to the
done label to preserve an errno or into the path that clobbers errno.
Clobbering errno for case #2 above would seem to be wrong, as we did not
actually search -- this would seem to be what POSIX expects, as well,
based on expectations of the conformance test suite.

Simplify reasoning about the two paths by splitting out an execvPe_prog
that does the execve(2) call specifically, and returns based on whether
the error would be fatal in a PATH search or not. For the
relative/absolute case, we can just ignore the return value and keep
errno intact. The search case gets simplified to return early if
we hit a fatal error, or continue until the end and clobber errno if
we did not find a suitable candidate.

Another posix_spawnp() test is added to confirm that we didn't break our
EACCES behavior in the process.

Sponsored by: Klara, Inc.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 65873
Build 62756: arc lint + arc unit

Event Timeline

des added a subscriber: des.
des added inline comments.
lib/libc/tests/gen/posix_spawn_test.c
137

I can haz description?

162

or just use atf_utils_create_file() instead

This revision is now accepted and ready to land.Wed, Jul 30, 11:30 PM
lib/libc/tests/gen/posix_spawn_test.c
162

that is absolutely brilliant, I love it. thanks!

markj added inline comments.
lib/libc/gen/exec.c
166

and below

184

AFAIK alloca() never fails or sets errno, so the new comment doesn't quite make sense to me.

lib/libc/gen/exec.c
184

Hmm, I didn't even bother checking. From the commit that added this (ce04fea4452e71a6da05a6b89fb0c020f02947a1):

  • If alloca() fails (can it?), set errno = ENOMEM explicitly.

Alright, Peter doesn't sound so sure, either. Our ARM implementation would return *something* and I guess we'd just get signalled on access if we blew past our stack limit. Reflecting a bit, yeah, I could see where it wouldn't make sense for alloca() to be able to fail in an obvious manner such as this. We can probably just axe this branch entirely?

lib/libc/gen/exec.c
184

Right, alloca() is basically "move the stack pointer down by n bytes and return sp + n", so shouldn't return NULL. If you run out of stack space, hopefully stack probing will cause a deterministic crash. (But see https://www.qualys.com/2017/06/19/stack-clash/stack-clash.txt )

assert that alloca() can't return NULL; we are pretty confident that it cannot,
but it seems wise to flag it if our worldview is destroyed.

This revision now requires review to proceed.Fri, Aug 1, 1:00 AM
This revision is now accepted and ready to land.Fri, Aug 1, 1:35 AM

None of our alloca implementations will return NULL. Years ago, emacs has an alloca built on top of malloc... <the horror>... But this guards adequately against that as well. None of our implementations ever have returned NULL. And yea, they are just sp -= X. One could construct a pathological case where they'd return 0, but none of that's relevant here (ans again the assert guards against that, though you'd have all kinds of other trouble well before that).

des added inline comments.
lib/libc/tests/gen/posix_spawn_test.c
159
lib/libc/gen/exec.c
306

the new paragraph is wrapped to a different width than the original one, please rewrap either one or the other (which could be a good occasion to fix the grammar in the first, which is)

uh, missing a couple of commas.

kevans added inline comments.
lib/libc/gen/exec.c
306
299                 /*                                                                                        
300                  * If the path is too long, then complain.  This is a possible                            
301                  * security issue: given a way to make the path too long, the                             
302                  * user may execute the wrong program.
This revision was automatically updated to reflect the committed changes.