[Mesa-dev] [PATCH] mesa: prevent deleting the dummy ATI_fs

[Miklós Máté]

On 03/11/17 11:04, Nicolai Hähnle wrote:
> On 03.11.2017 00:06, Miklós Máté wrote:
>> On 02/11/17 17:16, Nicolai Hähnle wrote:
>>> On 01.11.2017 00:34, Miklós Máté wrote:
>>>> This fixes a crash upon context destruction when
>>>> glGenFragmentShadersATI() was used. Backtrace:
>>>> ==15060== Invalid free() / delete / delete[] / realloc()
>>>> ==15060==    at 0x482F478: free (vg_replace_malloc.c:530)
>>>> ==15060==    by 0x57694F4: _mesa_delete_ati_fragment_shader 
>>>> (atifragshader.c:68)
>>>> ==15060==    by 0x58B33AB: delete_fragshader_cb (shared.c:208)
>>>> ==15060==    by 0x5838836: _mesa_HashDeleteAll (hash.c:295)
>>>> ==15060==    by 0x58B365F: free_shared_state (shared.c:377)
>>>> ==15060==    by 0x58B3BC2: _mesa_reference_shared_state (shared.c:469)
>>>> ==15060==    by 0x578687F: _mesa_free_context_data (context.c:1366)
>>>> ==15060==    by 0x595E9EC: st_destroy_context (st_context.c:642)
>>>> ==15060==    by 0x5987057: st_context_destroy (st_manager.c:772)
>>>> ==15060==    by 0x5B018B6: dri_destroy_context (dri_context.c:217)
>>>> ==15060==    by 0x5B006D3: driDestroyContext (dri_util.c:511)
>>>> ==15060==    by 0x4A1CBE6: dri3_destroy_context (dri3_glx.c:170)
>>>> ==15060==  Address 0x7b5dae0 is 0 bytes inside data symbol 
>>>> "DummyShader"
>>>>
>>>> This hasn't been an issue yet, because the only thing that calls
>>>> glGenFragmentShadersATI() is my WIP piglit test. SWKOTOR and Doom3
>>>> use hard-coded shader names.
>>>>
>>>> The patch also fixes this:
>>>> name=glGenFragmentShadersATI(1);
>>>> glDeleteFragmentShaderATI(name);
>>>>
>>>> Signed-off-by: Miklós Máté <mtmkls at gmail.com>
>>>> ---
>>>>   src/mesa/main/atifragshader.c | 4 +++-
>>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/mesa/main/atifragshader.c 
>>>> b/src/mesa/main/atifragshader.c
>>>> index 27d8b86477..fdfaea0528 100644
>>>> --- a/src/mesa/main/atifragshader.c
>>>> +++ b/src/mesa/main/atifragshader.c
>>>> @@ -60,6 +60,9 @@ void
>>>>   _mesa_delete_ati_fragment_shader(struct gl_context *ctx, struct 
>>>> ati_fragment_shader *s)
>>>>   {
>>>>      GLuint i;
>>>> +
>>>> +   if (s == &DummyShader)
>>>> +      return;
>>>> +
>>>>      for (i = 0; i < MAX_NUM_PASSES_ATI; i++) {
>>>>         free(s->Instructions[i]);
>>>>         free(s->SetupInst[i]);
>>>> @@ -295,7 +298,6 @@ _mesa_DeleteFragmentShaderATI(GLuint id)
>>>>         if (prog) {
>>>>        prog->RefCount--;
>>>>        if (prog->RefCount <= 0) {
>>>> -        assert(prog != &DummyShader);
>>>>               _mesa_delete_ati_fragment_shader(ctx, prog);
>>>
>>> This looks very much like it should be caught earlier by an error 
>>> check. Usually, the object ID 0 is silently ignored by glDelete* 
>>> commands (and negative values result in INVALID_VALUE).
>>>
>>> Cheers,
>>> Nicolai
>> It's not object ID 0. The DummyShader is used by 
>> GenFragmentShadersATI() as a placeholder to mark those IDs as 
>> allocated. DeleteFragmentShadersATI() and context cleanup should be 
>> able to remove the placeholders without crashing.
>
> Is there a reason why GenFragmentShadersATI doesn't just allocate a 
> new object? Is there precedent for this elsewhere? Using the 
> DummyShader approach seems fragile to me.
>
> Besides, with the patch as-is, you'll still let RefCounts go to 
> negative values, which seems like a bad idea.
>
> Why can't you just allocate the object immediately like is done for 
> most (all?) other objects, like textures or (GLSL) shaders instead of 
> adding hacks upon hacks?
>
I don't know. This is not my code, I'm just trying to fix things with 
minimal changes. ATI_fragment_shader is the programming language of 
r200, but I have no such hardware for testing any major change. I can 
only test on swrast, softpipe, llvmpipe and radeonsi.

If it's OK to make major changes that improve spec conformance, but may 
break r200, I can do that. My WIP piglit test series already uncovered a 
few conformance issues, and I'm sure there will be more of those as I 
make more tests.

I have a few questions about testing:
Am I right to assume that everything is legal that is not explicitly 
forbidden in the specification?
If a test detects an error, should it terminate immediately or continue 
and return fail at the end?
Should I post my piglit tests or my mesa changes first? My test series 
is about 15% complete now.

MM


>
>>
>> I'll resend the patch with better commit message. I don't know why it 
>> doesn't apply for Marek though, this file hasn't been touched in months.
>>
>> MM
>>
>>>
>>>
>>>>        }
>>>>         }
>>>>
>>>
>>>
>>
>
>