Add MAP_GROWSDOWN flag for call to mmap in Linux for stack auto-resizing

[Peter-Levine]

#1274 was, I believe, closed prematurely. The downstream bug was closed as it is being patched but I'm hoping that you would consider applying the necessary change upstream. Currently, only one page of memory is allocated to clone() in ExecDeathTestSpawnChild(). On my system, a page is 4 KB. Examples I've seen show 1MB as a reasonable stack size for clone(). In this PR, I have chosen getpagesize() * 8 since 8 is the first multiple of 2 that, when multiplied by the page size, provides enough stack space to prevent a stack overflow in a sandboxed environment.

[gennadiycivil]

252407455

[gennadiycivil]

@Peter-Levine thank you for this change. Could you please provide a bit more information:

Currently, only one page of memory is allocated to clone() in ExecDeathTestSpawnChild(). On my system, a page is 4 KB. ...

Since this seems to be working for a majority of cases ( I assume since no one said otherwise :-) ) what are the differences between "your system" and "not your system"?
Just trying to get a bit more info to see if we can dig into this.

Thanks again

[Peter-Levine]

@gennadiycivil It's understandable. Gentoo is a source-based distribution that primarily uses sandbox during the phases of building, testing, and installing packages to ensure that the local filesystem isn't accessed or modified and that no impermissible binaries are run during such phases. One of the methods employed is the use of LD_PRELOAD to preload libsandbox.so which defines various libc filesystem related syscall wrapper function's symbols that might access, modify, or execute files of a given path, such as execve. In a call to execve, the sandboxed function is called instead. It does some checks to ensure the given path is permissible and, if so, calls the intended libc function/syscall or, if not, denies the call and/or kills the process. It is implemented by reserving arrays of PATH_MAX chars on the stack.

On my system, PATH_MAX is equal to the page size. Because ExecDeathTestSpawnChild is granted only a page of memory, when it calls ExecDeathTestChildMain, and eventually execve, the libsandbox wrapper function is called and the stack is quickly exhausted.

Note, there is, to my knowledge, no other instance of a process failing with sandbox due to a stack overflow. It is, admittedly, a problem specific to sandbox which is predominantly used in Gentoo Linux.

It's your call. I can maintain the patch for the duration that I maintain the package downstream.

[Peter-Levine]

@gennadiycivil So, I looked into this a bit further. In Linux, when a process runs out of stack space it gets resized as needed. A thread, with its own stack, is still supposed to behave in a similar fashion. I found this entry in the mmap Linux man page:

MAP_GROWSDOWN
This flag is used for stacks. It indicates to the kernel virtual memory system that the mapping should extend downward in memory. The return address is one page lower than the memory area that is actually created in the process's virtual address space. Touching an address in the "guard" page below the mapping will cause the mapping to grow by a page. This growth can be repeated until the mapping grows to within a page of the high end of the next lower mapping, at which point touching the "guard" page will result in a SIGSEGV signal.

This flag is currently not included in the call to mmap that's used to initialize the stack for clone. When including it, I can confirm that it fixes the segfault. I'll revise the PR in the next few days to include it when building for Linux. It's a much more correct solution than increasing the stack size.

[gennadiycivil]

@Peter-Levine thank you for the changes. Would you be able to re-base?

[Peter-Levine]

@gennadiycivil Done.

[gennadiycivil]

253622730

[Peter-Levine]

253622730

Sorry, I'm not as familiar with git as some others. I rebased to master HEAD. Is 253622730 a Google Piper rev-id?

[Peter-Levine]

Updated code to keep lines under 80 character.

[gennadiycivil]

@Peter-Levine would it be possible to add a reproduction test? Since gentoo is using sandbox it seems plausible to add this exact scenario as one of the travis CI builds. What do you think?

[gennadiycivil]

We are actually hoping that while you work on setting up the repro CI test you would disover that your sandbox can be set up differently and would make this change not needed. Perhaps your sandbox is set up in such as way that limits virtual memory available?

[Peter-Levine]

@gennadiycivil I'll take a look at it in the next few days and see if I can reproduce it in Travis CL.

[Peter-Levine]

We are actually hoping that while you work on setting up the repro CI test you would disover that your sandbox can be set up differently and would make this change not needed. Perhaps your sandbox is set up in such as way that limits virtual memory available?

The problem is that the sandbox is run in the same address space and with the same stack as the calling process. Upon further observation, it seems that the issue is reproducible in the portage build environment when the usersandbox FEATURE flag is set (implying sandbox and the called process are to be run as an unprivileged user) and during invocation SANDBOX_ACTIVE is set in the environment, presumably by a previous invocation of sandbox, to prevent an unnecessary parallel invocation of sandbox. In such a scenario, the page of stack is exhausted shortly after the call to clone().

Regardless, I suppose it would be wrong to lay at upstream's doorstep a workaround for an outdated, largely unmaintained, and likely insecure Gentoo related library. MAP_GROWSDOWN comes with its own potential security issues and I would assume that increasing the stack size might interfere with googletest's usage in memory-constrained environments. The patch is simple enough to maintain downstream until a better solution is found. Closing the PR.

[sfc-gh-bhannel]

I'm encountering this same issue running on CentOS 7 and compiling with gcc 10.0.2 with the -fstack-check flag. I agree that 2 pages for the stack seems very small. I also don't entirely understand why the default behavior is to use clone, rather than fork, since fork is available on more platforms and does not have this issue with needing a newly allocated stack. The comment on --death_test_use_fork says that forking is not recommended, but doesn't specify why.