[Piglit] [PATCH] mingw: use default 2MB stack size instead of 1MB

[Jose Fonseca]

On 12/10/17 20:19, Brian Paul wrote:
> On 10/12/2017 12:11 PM, Jose Fonseca wrote:
>> On 12/10/17 17:51, Brian Paul wrote:
>>> On 10/12/2017 08:04 AM, Jose Fonseca wrote:
>>>> The intent here was not so much to match the piglti MSVC build, but 
>>>> apps
>>>> build by MSVC in general.
>>>>
>>>> After all, nothing ever prevented us from setting a huge stack size on
>>>> both MinGW and MSVC alike, as both toolchains allow to congifure the
>>>> stack size to whatever we want.
>>>>
>>>>
>>>> The key issue here is that OpenGL driver don't get to pick the apps 
>>>> they
>>>> are loaded, and real OpenGL applications will be likely built with MSVC
>>>> instead of Mingw, and therefore will likely only have the MSVC default
>>>> stack size.  And we should err on the side of caution when testing.
>>>>
>>>>
>>>> Regardless of the compiler used, if we bump the stack size in piglit,
>>>> one is just increasing the chance that wasteful stack allocations go
>>>> undetected on piglit and blow up on real applications.
>>>>
>>>>
>>>> Therefore I suggest we continue to keep 1MB default, and try to fix 
>>>> Mesa
>>>> to be less stack hungry.   If that's not pratical here,
>>>
>>> The ir_expression::constant_expression_value() function's local vars
>>> are pretty minimal.  The two that stand out:
>>>
>>>     ir_constant *op[ARRAY_SIZE(this->operands)] = { NULL, };
>>>     ir_constant_data data;
>>>
>>> are only 32 and 128 bytes, respectively (on a 32-bit build).  I'm not
>>> sure what else accounts for the approx 2KB of the activation record.
>>>
>>> I don't see an obvious way to fix the problem.  Even if we could
>>> reduce per-call stack memory, someone could write an evil shader that
>>> adds a thousand or more terms and we'd overflow the stack again.
>>
>> I'm not following...
>>
>> If the application is malicous, it can overflow the stack without OpenGL
>> driver help.  We have to trust the application.  After all, the opengl
>> driver is in the same process.
>>
>> If the application is a browser, who needs to handle untrusted shaders,
>> then it's the application responsibility to ensure it has enough stack
>> to cope with big shaders.
>>
>> And for the sake of argument, if increasing stack size is the solution
>> for malicious shaders, where would one stop?  If increasing stack size
>> is not a solution to malicous shaders, then why is it relevant to the
>> discussion?
> 
> I'm just saying that if we found a way to "fix Mesa to be less stack 
> hungry" someone could generate a new "evil" Piglit test that'd still 
> breaks things.  Any construct which generates a really deep IR tree and 
> is evaluated recursively could run out of stack space.

Right.  Compiling huge shaders will consume resources (stack, memory, 
cpu), and it's unavoidable.  And if there's no limit on then shader 
string size, then there's equally no bound on how much resources will be 
needed to compile the shaders.

Still, we want to be frugal on stack memory consumption within reason, 
as it's immutable once the process is created -- it can't be stretched. 
Plus we have defaults that were set decades ago.

> I wasn't thinking about it, but now that you mention malicious shaders, 
> we should probably consider the case of the GLSL compiler running in a 
> driver loaded by the X server for indirect rendering.  But that's a 
> whole other topic.

It should be possible to ensure there is no remote exploits, but it's 
really hard (impossible?) to prevent DoS with graphics APIs (GLX, 
WebGL), particularly when one has shaders.

>>>> then we should
>>>> try to bump the stack size of specific piglit tests (like those that
>>>> stress the compiler to the extreme, as Cmake allows to set these 
>>>> options
>>>> per executable), and only those tests.  Or just mark the affected tests
>>>> as expected fail/skip.
>>>
>>> The op-selection-bool-bvec4-bvec4.frag test is run with
>>> shader_runner.exe.  I think most of the large/complex shaders in
>>> Piglit are run with shader_runner so I don't think it makes much
>>> difference if only shader_runner or all piglit executables are build
>>> with a 2MB stack.
>>>
>>> In any case, I've got a patch which only sets shader_runner's stack
>>> size which I'll post next.
>>>
>>> I guess another option is to change the
>>> op-selection-bool-bvec4-bvec4.frag test to be simpler, but that's just
>>> sweeping the root problem under the rug.  Someone could always craft a
>>> complex shader which blows out the stack.
>>
>> That's fine.  I'm not worried about uber shaders at all.  Malicous or 
>> not.
>>
>> What I'm worried is that once you set the stack to 2MB, some appearently
>> innocuous code somewhere in Mesa source tree (probably completely
>> unrelated to GLSL for example) wastefully uses up 1MB of stack, and
>> piglit does not catch the issue because all tests have 2MB of stack.
>>
>> And this is not hypothetical: we had several functions in Mesa related
>> to writing/read/converting pixels to/from textures, eating up wasteful
>> amounts of stack.  And more than once real apps hit stack overflow
>> because of it.  I'd hate to see us increasing the stack sizes of all our
>> tests, just for the same issues with real applications creeping back in.
>>
>> The only way I see to avoid this sort of regressions is to continue
>> using the default stack, and use big stacks only when necessary, IMO.
> 
> So, is increasing the stack size for shader_runner alone to 2MB 
> acceptable to you?

Yes, that's a great compromise IMO.  We can still run this test, but 
still catch if other tests start using more stack than it should be 
necessary.

> Otherwise, the question is: can we change the default stack size at 
> shader_runner runtime for particular shader tests?  I guess 
> setrlimit(RLIMIT_STACK) is an option on Linux.  Not sure about Windows.

The only way I know to change the stack size on Windows is to create a 
new thread.

That is, in main(), one would need to: CreateThread w/ desired size, run 
the tests on that thread, and have the main thread wait for it and 
return the exit status.

Jose