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

[Roland Scheidegger]

Am 12.10.2017 um 21:19 schrieb Brian Paul:
> 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.

I think to be safe there, it would be necessary for the compiler to
detect this and refuse compilation (once some recursive call limit is
reached). (But of course not intentionally malicious shaders should
compile if they are at least semi-reasonable.)
Not sure though if that's practical... (I don't know if that piglit test
is really "sane" albeit it doesn't look like it's really meant to stress
this in particular, so it probably would be nice if it would just work.)

Roland


> 
> 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.
> 
> 
>>
>>>> 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?
> 
> 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.
> 
> -Brian
> 
> _______________________________________________
> Piglit mailing list
> Piglit at lists.freedesktop.org
> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_piglit&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0&m=j_R6Mjocs1ThArAwSh265ZRfCovl_j2IYzOCYhpxGXc&s=32kpFq65LTyWpkYj5u7cTLV8VjYCwXWoJYJ9hvGHr2Y&e=
> 
>