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

[Miklós Máté]

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.

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

>
>
>>        }
>>         }
>>
>
>