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

[Nicolai Hähnle]

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?

Cheers,
Nicolai

> 
> 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
> 
>>
>>
>>>        }
>>>         }
>>>
>>
>>
> 


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.