Details

Reviewers

kib
jilles
andrew_tao173.riddles.org.uk

Group Reviewers

secteam
releng

Commits

rS361996: execvPe: obviate the need for potentially large stack allocations

Summary

Some environments in which execvPe may be called have a limited amount of stack available. Currently, it avoidably allocates a segment on the stack large enough to hold PATH so that it may be mutated and use strsep() for easy parsing. This logic is now rewritten to just operate on the immutable string passed in and do the necessary math to extract individual paths, since it will be copying out those segments to another buffer anyways and piecing them together with the name for a full path.

Additional size is also needed for the stack in posix_spawnp(), because it may need to push all of argv to the stack and rebuild the command with sh in front of it. We'll make sure it's properly aligned for the new thread, but future work should likely make rfork_thread a little easier to use by ensuring proper alignment.

Some trivial cleanup has been done with a couple of error writes, moving strings into char arrays for use with the less fragile sizeof().

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kevans requested review of this revision.May 28 2020, 4:58 PM

kevans created this revision.

kevans created this object with visibility "Custom Policy".

Harbormaster completed remote builds in B31348: Diff 72365.May 28 2020, 4:58 PM

Adding releng. They would deal with in-progress releases, not secteam. We get to deal with them *after* release. :-)

kib accepted this revision.May 28 2020, 6:21 PM

kib added inline comments.

lib/libc/gen/exec.c
209 ↗	(On Diff #72365)	I think this line (and 206) can be improved by moving the string to const array and using sizeof().

This revision is now accepted and ready to land.May 28 2020, 6:21 PM

andrew_tao173.riddles.org.uk accepted this revision.May 28 2020, 6:57 PM

Address the second alloca() that could be a problem if _execve fails with ENOEXEC and we happened to have a lot of argv. This is a little harder to solve with a local write in execvPe without forcing extra work on other consumers, so just make sure we have enough space for our original 4k + whatever could be allocated there in the worst case.

While we're here, push some constants+magic out into a const array for a drive-by improvement.

This revision now requires review to proceed.May 28 2020, 8:12 PM

Harbormaster completed remote builds in B31354: Diff 72373.May 28 2020, 8:12 PM

jilles requested changes to this revision.May 28 2020, 8:50 PM

jilles added inline comments.

lib/libc/gen/exec.c
160 ↗	(On Diff #72365)	There needs to be `op = NULL;` here so that the path search loop is not entered with uninitialized `op` if the `_execve()` fails with `ENOENT` or a similar error.
234 ↗	(On Diff #72365)	This is an `alloca()` as well, for which the stack allocated by `do_posix_spawn()` may not be sufficient. Perhaps `do_posix_spawnp()` should reserve enough space for this allocation if it should happen.

This revision now requires changes to proceed.May 28 2020, 8:50 PM

kevans marked an inline comment as done.May 28 2020, 9:51 PM

kevans added inline comments.

lib/libc/gen/exec.c
160 ↗	(On Diff #72365)	Good catch, will fix
234 ↗	(On Diff #72365)	There is a follow-up update that did include this. =)

andrew_tao173.riddles.org.uk added inline comments.May 28 2020, 11:56 PM

lib/libc/gen/exec.c
183 ↗	(On Diff #72373)	On second look this logic looks like it could be a lot simpler; as far as I can tell the two blocks for leading/trailing/double colon can be merged, and so can the ones for final and non-final components: something like, if (np == op) { /* empty component: / p = "."; lp = 1; } else { / non-empty component: */ p = op; lp = np - op; } or am I missing something?

kevans added inline comments.May 29 2020, 12:16 AM

lib/libc/gen/exec.c
183 ↗	(On Diff #72373)	Whoops- looks like I forgot to re-evaluate and simplify after `strchr` -> `strchrnul`

Restore the terminal condition for name = absolute/relative path
Simplify logic; strchrnul will do exactly what we want

Harbormaster completed remote builds in B31360: Diff 72387.May 29 2020, 1:00 AM

andrew_tao173.riddles.org.uk added inline comments.May 29 2020, 3:02 AM

lib/libc/gen/posix_spawn.c
291 ↗	(On Diff #72387)	Does rfork_thread align the stack properly, or does it assume the caller did that?

jilles added inline comments.May 29 2020, 1:13 PM

lib/libc/gen/posix_spawn.c
291 ↗	(On Diff #72387)	On amd64, `rfork_thread()` loads the `stack` parameter into `%rsp` and then performs a call, so the stack will be properly aligned if `stack` is. Ensuring the stack size is a multiple of 16 and using `aligned_alloc()` will suffice. However, on i386, `rfork_thread()` pushes 4 bytes onto `stack` and then performs a call, so doing the obvious thing will lead to a misaligned stack.

Ensure 2-byte alignment of the stack. For the non-spawnp case, we're using _RFORK_THREAD_STACK_SIZE without modifications so I've dropped a _Static_assert and comment near it to make it clear that any bumps should take into account the alignment requirements rather than bothering with any extra math for the easy case where it's fixed at compile-time anyways.

Harbormaster completed remote builds in B31373: Diff 72412.May 29 2020, 4:03 PM

In D25038#551749, @kevans wrote:

Ensure 2-byte alignment of the stack. For the non-spawnp case, we're using _RFORK_THREAD_STACK_SIZE without modifications so I've dropped a _Static_assert and comment near it to make it clear that any bumps should take into account the alignment requirements rather than bothering with any extra math for the easy case where it's fixed at compile-time anyways.

I think that alignment should be done in rfork_thread() instead, if any. So that it would be set for all users, not just this place.

andrew_tao173.riddles.org.uk added inline comments.May 29 2020, 4:43 PM

lib/libc/gen/posix_spawn.c
291 ↗	(On Diff #72387)	We don't actually provide or assume any better than 4-byte stack alignment on i386, this is a source of issues with GCC-compiled ports that I've brought up in the past to no avail.

In D25038#551763, @kib wrote:

In D25038#551749, @kevans wrote:

Ensure 2-byte alignment of the stack. For the non-spawnp case, we're using _RFORK_THREAD_STACK_SIZE without modifications so I've dropped a _Static_assert and comment near it to make it clear that any bumps should take into account the alignment requirements rather than bothering with any extra math for the easy case where it's fixed at compile-time anyways.

I think that alignment should be done in rfork_thread() instead, if any. So that it would be set for all users, not just this place.

I have mixed feelings about doing it in rfork_thread() vs. having rfork_thread() reject misaligned stack up-front with EINVAL and forcing the caller to be explicit about what they're doing. I don't know how rfork_thread gets used in practice outside of this one instance that it's used in base given that it's been discouraged for some years, but it feels somewhat wrong to move the stack pointer out from underneath the caller like this while not knowing anything about the shape/size of the stack or intent of the caller.

Adding an EINVAL return to rfork_thread will cause this code to fall through to the "use vfork instead" logic, hiding the error in unexpected ways.

I don't think rfork_thread should make any promises regarding the initial stackframe of the child beyond the fact of following the platform ABI for a function with a single void* parameter. So I would say that rfork_thread should do any required alignment.

While at it, would it be worth adding a check that the stack size is at least MINSIGSTKSZ + PATH_MAX ?

In D25038#551786, @andrew_tao173.riddles.org.uk wrote:

While at it, would it be worth adding a check that the stack size is at least MINSIGSTKSZ + PATH_MAX ?

When you say 'adding a check', are you proposing a static assert that PSPAWN_STACK_ALIGNMENT at least fits MINSIGSTKSZ + PATH_MAX , or runtime check just before calling rfork_thread()?

In D25038#551788, @kevans wrote:

In D25038#551786, @andrew_tao173.riddles.org.uk wrote:

While at it, would it be worth adding a check that the stack size is at least MINSIGSTKSZ + PATH_MAX ?

When you say 'adding a check', are you proposing a static assert that PSPAWN_STACK_ALIGNMENT at least fits MINSIGSTKSZ + PATH_MAX , or runtime check just before calling rfork_thread()?

either a static assert that _RFORK_THREAD_STACK_SIZE is big enough, or simply define _RFORK_THREAD_STACK_SIZE as MINSIGSTKSZ + PATH_MAX aligned to the required alignment.

In D25038#551763, @kib wrote:

In D25038#551749, @kevans wrote:

Ensure 2-byte alignment of the stack. For the non-spawnp case, we're using _RFORK_THREAD_STACK_SIZE without modifications so I've dropped a _Static_assert and comment near it to make it clear that any bumps should take into account the alignment requirements rather than bothering with any extra math for the easy case where it's fixed at compile-time anyways.

I think that alignment should be done in rfork_thread() instead, if any. So that it would be set for all users, not just this place.

Does it have to be done in this patch, or can I follow-up after the imminent issue is fixed?

In D25038#551854, @kevans wrote:

In D25038#551763, @kib wrote:

In D25038#551749, @kevans wrote:

Ensure 2-byte alignment of the stack. For the non-spawnp case, we're using _RFORK_THREAD_STACK_SIZE without modifications so I've dropped a _Static_assert and comment near it to make it clear that any bumps should take into account the alignment requirements rather than bothering with any extra math for the easy case where it's fixed at compile-time anyways.

I think that alignment should be done in rfork_thread() instead, if any. So that it would be set for all users, not just this place.

Does it have to be done in this patch, or can I follow-up after the imminent issue is fixed?

Have ? The issue is clearly unrelated, and even if you fixed it there, it needs a separate commit.

In D25038#551862, @kib wrote:

In D25038#551854, @kevans wrote:

In D25038#551763, @kib wrote:

I think that alignment should be done in rfork_thread() instead, if any. So that it would be set for all users, not just this place.

Does it have to be done in this patch, or can I follow-up after the imminent issue is fixed?

Have ? The issue is clearly unrelated, and even if you fixed it there, it needs a separate commit.

Ah, sorry- the "instead, if any" chunk of your original comment lead me to believe that you'd prefer it get done here and now rather than after the fact.

What remains to be done to push this forward?

Will we be able to wrap this up before the conclusion of the 11.4 release cycle, or is this suddenly going to get bumped into SA status?

It's also not clear to me if we're now-satisfied with the changes, including stack alignment.

It can still be fixed before the release (if appropriate) with approval from so@. Otherwise, it will be a post-release follow-up item.

In D25038#553896, @kevans wrote:

Will we be able to wrap this up before the conclusion of the 11.4 release cycle, or is this suddenly going to get bumped into SA status?

It's also not clear to me if we're now-satisfied with the changes, including stack alignment.

I've verified that as long as stack+stacksz is a multiple of 16 bytes, the child thread will have the expected stack alignment, so this looks right to me.

andrew_tao173.riddles.org.uk added inline comments.Jun 5 2020, 10:50 AM

lib/libc/gen/exec.c
236 ↗	(On Diff #72412)	There's another bug here, though I don't think it's a security issue; `cnt` can be 0, which means that the `memp` array doesn't get terminated, and execve runs off the end of it and passes garbage args to the invoked script. Maybe add 3 rather than 2 to `cnt` above, and have: if (cnt > 0) bcopy(argv + 1, memp + 2, cnt * sizeof(char*)); else memp[2] = NULL; `

andrew_tao173.riddles.org.uk requested changes to this revision.Jun 5 2020, 3:15 PM

andrew_tao173.riddles.org.uk added inline comments.

lib/libc/gen/exec.c
205 ↗	(On Diff #72412)	This "continue" is wrong now that the advancement of the loop pointer has been moved to below it - it'll loop forever

This revision now requires changes to proceed.Jun 5 2020, 3:15 PM

Move advancement of op up to just after its use -- I suspect a previous iteration was still using op and I overlooked moving this up.

Fix the argv passed to execve(2) in the ENOEXEC fallback case and add some best-effort tests. argv[0] == NULL is not explicitly forbidden, so we should attempt to cope with it and at least terminate memp properly if we get it. The spec for execve seems to indicate that we should have been preserving argv[0] all along, but in the argv[0] == NULL edge-case we retain the old behavior of using "sh" because it'd be somewhat fragile otherwise. For reference:

In the cases where the other members of the exec family of functions would fail and set errno to [ENOEXEC], the execlp() and execvp() functions shall execute a command interpreter and the environment of the executed command shall be as if the process invoked the sh utility using execl() as follows:

execl(<shell path>, arg0, file, arg1, ..., (char *)0);

where <shell path> is an unspecified pathname for the sh utility, file is the process image file, and for execvp(), where arg0, arg1, and so on correspond to the values passed to execvp() in argv[0], argv[1], and so on.

I've conflated this with the pre-existing security isue since we need to allocate stack space for it anyways, and it seems like we should fix that detail as well. It could easily go in beforehand.

Harbormaster completed remote builds in B31508: Diff 72718.Jun 5 2020, 4:28 PM

I think this addresses all of my points.

I'm going to take the silence as acceptance... secteam, can I please get approval to proceed to commit and MFC this all to the way to releng/11.4?

kevans retitled this revision from execvPe: obviate the need for environment-controlled stack allocation to execvPe: obviate the need for potentially large stack allocations.Jun 9 2020, 9:02 PM

kevans edited the summary of this revision. (Show Details)

kib accepted this revision.Jun 9 2020, 9:19 PM

jilles accepted this revision.Jun 9 2020, 9:21 PM

This revision is now accepted and ready to land.Jun 9 2020, 9:21 PM

Per discussion with Kyle offline, we are going to defer this change until after the 11.4 release. We are just in a tough spot to inject a change of this size this late into the release process.

That said, I'm accepting the change (note, this is not a technical review) as secteam with the expectation that others (@kib, @jilles) will help review the latest patch and ensure we are good to go. (Which appears to already have happened.)

Thanks, folks. I'll aim to commit this tonight in two parts (ENOEXEC bug and then the rest) with an MFC after: 1 week

kevans changed the visibility from "Custom Policy" to "Public (No Login Required)".Jun 10 2020, 1:16 AM

Herald added a subscriber: imp. · View Herald TranscriptJun 10 2020, 1:16 AM

Closed by commit rS361996: execvPe: obviate the need for potentially large stack allocations (authored by kevans). · Explain WhyJun 10 2020, 1:30 AM

This revision was automatically updated to reflect the committed changes.

kevans mentioned this in rS361995: execvp: fix up the ENOEXEC fallback.

kevans added a commit: rS361996: execvPe: obviate the need for potentially large stack allocations.

kevans mentioned this in rS362281: MFC r361995-r361996, r361999, r362111: posix_spawnp fixes.Jun 18 2020, 9:25 AM

execvPe: obviate the need for potentially large stack allocations
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 72907

head/lib/libc/gen/exec.c

head/lib/libc/gen/posix_spawn.c

execvPe: obviate the need for potentially large stack allocationsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 72907

head/lib/libc/gen/exec.c

head/lib/libc/gen/posix_spawn.c

execvPe: obviate the need for potentially large stack allocations
ClosedPublic
Actions

Revision Contents
Changeset List