[Mesa-dev] [PATCH 2/3] mesa: Verify memory allocations success in _mesa_PushClientAttrib

[Ian Romanick]

On 12/03/2013 01:24 PM, Juha-Pekka Heikkilä wrote:
> On Tue, Dec 3, 2013 at 6:42 PM, Brian Paul <brianp at vmware.com> wrote:
>> On 12/02/2013 02:39 AM, Juha-Pekka Heikkila wrote:
>>>
>>> Check if any of the callocs fail and report it with _mesa_error
>>> if needed.
>>>
>>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
>>> ---
>>>   src/mesa/main/attrib.c | 34 ++++++++++++++++++++++++++++++----
>>>   1 file changed, 30 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/src/mesa/main/attrib.c b/src/mesa/main/attrib.c
>>> index c9332bd..2418fb0 100644
>>> --- a/src/mesa/main/attrib.c
>>> +++ b/src/mesa/main/attrib.c
>>> @@ -1488,6 +1488,12 @@ init_array_attrib_data(struct gl_context *ctx,
>>>   {
>>>      /* Get a non driver gl_array_object. */
>>>      attrib->ArrayObj = CALLOC_STRUCT( gl_array_object );
>>> +
>>> +   if (attrib->ArrayObj == NULL) {
>>> +      _mesa_error(ctx, GL_OUT_OF_MEMORY, "glPushClientAttrib");
>>> +      return;
>>> +   }
>>> +
>>
>>
>> This is good, but if CALLOC_STRUCT() fails we'd crash anyway because the
>> following call to save_array_attrib() would dereference the null pointer.
>> init_array_attrib_data() should probably return a true/false success/failure
>> result so the following array attrib calls could be skipped.
> 
> I guess you are right, I'll change this to return boolean. This

Pre-emptive review comment: Use bool / true / false from stdbool.h. :)

> function is called just from one place and I did add check that
> attrib->ArrayObj is really in place to avoid the crash but that is
> just silly hiding of functionality. My idea with checking
> attrib->ArrayObj against NULL as detecting error was just to get out
> from _mesa_PushClientAttrib easy and quick.
> 
>>>      _mesa_initialize_array_object(ctx, attrib->ArrayObj, 0);
>>>   }
>>>
>>> @@ -1516,7 +1522,7 @@ _mesa_PushClientAttrib(GLbitfield mask)
>>>      GET_CURRENT_CONTEXT(ctx);
>>>
>>>      if (ctx->ClientAttribStackDepth >= MAX_CLIENT_ATTRIB_STACK_DEPTH) {
>>> -      _mesa_error( ctx, GL_STACK_OVERFLOW, "glPushClientAttrib" );
>>> +      _mesa_error(ctx, GL_STACK_OVERFLOW, "glPushClientAttrib");
>>>         return;
>>>      }
>>>
>>> @@ -1529,10 +1535,19 @@ _mesa_PushClientAttrib(GLbitfield mask)
>>>         struct gl_pixelstore_attrib *attr;
>>>         /* packing attribs */
>>>         attr = CALLOC_STRUCT( gl_pixelstore_attrib );
>>> +      if (attr == NULL) {
>>> +         _mesa_error(ctx, GL_OUT_OF_MEMORY, "glPushClientAttrib");
>>> +         goto end;
>>> +      }
>>>         copy_pixelstore(ctx, attr, &ctx->Pack);
>>>         save_attrib_data(&head, GL_CLIENT_PACK_BIT, attr);
>>>         /* unpacking attribs */
>>>         attr = CALLOC_STRUCT( gl_pixelstore_attrib );
>>> +      if (attr == NULL) {
>>> +         _mesa_error(ctx, GL_OUT_OF_MEMORY, "glPushClientAttrib");
>>> +         goto end;
>>> +      }
>>> +
>>>         copy_pixelstore(ctx, attr, &ctx->Unpack);
>>>         save_attrib_data(&head, GL_CLIENT_UNPACK_BIT, attr);
>>>      }
>>> @@ -1540,13 +1555,24 @@ _mesa_PushClientAttrib(GLbitfield mask)
>>>      if (mask & GL_CLIENT_VERTEX_ARRAY_BIT) {
>>>         struct gl_array_attrib *attr;
>>>         attr = CALLOC_STRUCT( gl_array_attrib );
>>> +      if (attr == NULL) {
>>> +         _mesa_error(ctx, GL_OUT_OF_MEMORY, "glPushClientAttrib");
>>> +         goto end;
>>> +      }
>>> +
>>>         init_array_attrib_data(ctx, attr);
>>> +      if (attr->ArrayObj == NULL) {
>>> +          goto end;
>>> +      }
>>> +
>>>         save_array_attrib(ctx, attr, &ctx->Array);
>>>         save_attrib_data(&head, GL_CLIENT_VERTEX_ARRAY_BIT, attr);
>>>      }
>>> -
>>> -   ctx->ClientAttribStack[ctx->ClientAttribStackDepth] = head;
>>> -   ctx->ClientAttribStackDepth++;
>>> +end:
>>> +   if (head != NULL) {
>>> +       ctx->ClientAttribStack[ctx->ClientAttribStackDepth] = head;
>>> +       ctx->ClientAttribStackDepth++;
>>> +   }
>>>   }
>>
>>
>> The rest looks OK.
>>
>> -Brian
>>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev