mesa: remove usage of alloca in externalobjects.c v3

Submitted by Andres Rodriguez on Jan. 31, 2018, 7:03 p.m.

Details

Message ID 20180131190334.5668-1-andresx7@gmail.com
State New
Headers show
Series "mesa: remove usage of alloca in externalobjects.c" ( rev: 3 ) in Mesa

Not browsing as part of any series.

Commit Message

Andres Rodriguez Jan. 31, 2018, 7:03 p.m.
Don't want an overly large numBufferBarriers/numTextureBarriers to blow
up the stack.

v2: handle malloc errors
v3: fix patch

Suggested-by: Emil Velikov <emil.velikov@collabora.com>
Signed-off-by: Andres Rodriguez <andresx7@gmail.com>
---
 src/mesa/main/externalobjects.c | 48 +++++++++++++++++++++++++++++++++++------
 1 file changed, 42 insertions(+), 6 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/mesa/main/externalobjects.c b/src/mesa/main/externalobjects.c
index 463debd268..a28d6dba6f 100644
--- a/src/mesa/main/externalobjects.c
+++ b/src/mesa/main/externalobjects.c
@@ -713,91 +713,127 @@  _mesa_WaitSemaphoreEXT(GLuint semaphore,
                        const GLuint *buffers,
                        GLuint numTextureBarriers,
                        const GLuint *textures,
                        const GLenum *srcLayouts)
 {
    GET_CURRENT_CONTEXT(ctx);
    struct gl_semaphore_object *semObj;
    struct gl_buffer_object **bufObjs;
    struct gl_texture_object **texObjs;
 
+   const char *func = "glWaitSemaphoreEXT";
+
    if (!ctx->Extensions.EXT_semaphore) {
-      _mesa_error(ctx, GL_INVALID_OPERATION, "glWaitSemaphoreEXT(unsupported)");
+      _mesa_error(ctx, GL_INVALID_OPERATION, "%s(unsupported)", func);
       return;
    }
 
    ASSERT_OUTSIDE_BEGIN_END(ctx);
 
    semObj = _mesa_lookup_semaphore_object(ctx, semaphore);
    if (!semObj)
       return;
 
    FLUSH_VERTICES(ctx, 0);
    FLUSH_CURRENT(ctx, 0);
 
-   bufObjs = alloca(sizeof(struct gl_buffer_object **) * numBufferBarriers);
+   bufObjs = malloc(sizeof(struct gl_buffer_object **) * numBufferBarriers);
+   if (!bufObjs) {
+      _mesa_error(ctx, GL_OUT_OF_MEMORY, "%s(numBufferBarriers=%u)",
+                  func, numBufferBarriers);
+      goto end;
+   }
+
    for (unsigned i = 0; i < numBufferBarriers; i++) {
       bufObjs[i] = _mesa_lookup_bufferobj(ctx, buffers[i]);
    }
 
-   texObjs = alloca(sizeof(struct gl_texture_object **) * numTextureBarriers);
+   texObjs = malloc(sizeof(struct gl_texture_object **) * numTextureBarriers);
+   if (!texObjs) {
+      _mesa_error(ctx, GL_OUT_OF_MEMORY, "%s(numTextureBarriers=%u)",
+                  func, numTextureBarriers);
+      goto end;
+   }
+
    for (unsigned i = 0; i < numTextureBarriers; i++) {
       texObjs[i] = _mesa_lookup_texture(ctx, textures[i]);
    }
 
    ctx->Driver.ServerWaitSemaphoreObject(ctx, semObj,
                                          numBufferBarriers, bufObjs,
                                          numTextureBarriers, texObjs,
                                          srcLayouts);
+
+end:
+   free(bufObjs);
+   free(texObjs);
 }
 
 void GLAPIENTRY
 _mesa_SignalSemaphoreEXT(GLuint semaphore,
                          GLuint numBufferBarriers,
                          const GLuint *buffers,
                          GLuint numTextureBarriers,
                          const GLuint *textures,
                          const GLenum *dstLayouts)
 {
    GET_CURRENT_CONTEXT(ctx);
    struct gl_semaphore_object *semObj;
    struct gl_buffer_object **bufObjs;
    struct gl_texture_object **texObjs;
 
+   const char *func = "glSignalSemaphoreEXT";
+
    if (!ctx->Extensions.EXT_semaphore) {
-      _mesa_error(ctx, GL_INVALID_OPERATION, "glSignalSemaphoreEXT(unsupported)");
+      _mesa_error(ctx, GL_INVALID_OPERATION, "%s(unsupported)", func);
       return;
    }
 
    ASSERT_OUTSIDE_BEGIN_END(ctx);
 
    semObj = _mesa_lookup_semaphore_object(ctx, semaphore);
    if (!semObj)
       return;
 
    FLUSH_VERTICES(ctx, 0);
    FLUSH_CURRENT(ctx, 0);
 
-   bufObjs = alloca(sizeof(struct gl_buffer_object **) * numBufferBarriers);
+   bufObjs = malloc(sizeof(struct gl_buffer_object **) * numBufferBarriers);
+   if (!bufObjs) {
+      _mesa_error(ctx, GL_OUT_OF_MEMORY, "%s(numBufferBarriers=%u)",
+                  func, numBufferBarriers);
+      goto end;
+   }
+
    for (unsigned i = 0; i < numBufferBarriers; i++) {
       bufObjs[i] = _mesa_lookup_bufferobj(ctx, buffers[i]);
    }
 
-   texObjs = alloca(sizeof(struct gl_texture_object **) * numTextureBarriers);
+   texObjs = malloc(sizeof(struct gl_texture_object **) * numTextureBarriers);
+   if (!texObjs) {
+      _mesa_error(ctx, GL_OUT_OF_MEMORY, "%s(numTextureBarriers=%u)",
+                  func, numTextureBarriers);
+      goto end;
+   }
+
    for (unsigned i = 0; i < numTextureBarriers; i++) {
       texObjs[i] = _mesa_lookup_texture(ctx, textures[i]);
    }
 
    ctx->Driver.ServerSignalSemaphoreObject(ctx, semObj,
                                            numBufferBarriers, bufObjs,
                                            numTextureBarriers, texObjs,
                                            dstLayouts);
+
+end:
+   free(bufObjs);
+   free(texObjs);
 }
 
 void GLAPIENTRY
 _mesa_ImportMemoryFdEXT(GLuint memory,
                         GLuint64 size,
                         GLenum handleType,
                         GLint fd)
 {
    GET_CURRENT_CONTEXT(ctx);
 

Comments

On 31 January 2018 at 19:03, Andres Rodriguez <andresx7@gmail.com> wrote:
> Don't want an overly large numBufferBarriers/numTextureBarriers to blow
> up the stack.
>
> v2: handle malloc errors
> v3: fix patch
>
> Suggested-by: Emil Velikov <emil.velikov@collabora.com>
> Signed-off-by: Andres Rodriguez <andresx7@gmail.com>
> ---
>  src/mesa/main/externalobjects.c | 48 +++++++++++++++++++++++++++++++++++------
>  1 file changed, 42 insertions(+), 6 deletions(-)
>
> diff --git a/src/mesa/main/externalobjects.c b/src/mesa/main/externalobjects.c
> index 463debd268..a28d6dba6f 100644
> --- a/src/mesa/main/externalobjects.c
> +++ b/src/mesa/main/externalobjects.c
> @@ -713,91 +713,127 @@ _mesa_WaitSemaphoreEXT(GLuint semaphore,
>                         const GLuint *buffers,
>                         GLuint numTextureBarriers,
>                         const GLuint *textures,
>                         const GLenum *srcLayouts)
>  {
>     GET_CURRENT_CONTEXT(ctx);
>     struct gl_semaphore_object *semObj;
>     struct gl_buffer_object **bufObjs;
>     struct gl_texture_object **texObjs;
Initialize texObjs (might as well do bufObjs) since on bufObjs
allocation failure we'll end up feeding junk to free().

> -   bufObjs = alloca(sizeof(struct gl_buffer_object **) * numBufferBarriers);
> +   bufObjs = malloc(sizeof(struct gl_buffer_object **) * numBufferBarriers);
> +   if (!bufObjs) {
> +      _mesa_error(ctx, GL_OUT_OF_MEMORY, "%s(numBufferBarriers=%u)",
> +                  func, numBufferBarriers);
> +      goto end;
> +   }
> +

> +end:
> +   free(bufObjs);
> +   free(texObjs);
>  }
>
>  void GLAPIENTRY
>  _mesa_SignalSemaphoreEXT(GLuint semaphore,
>                           GLuint numBufferBarriers,
>                           const GLuint *buffers,
>                           GLuint numTextureBarriers,
>                           const GLuint *textures,
>                           const GLenum *dstLayouts)
>  {
>     GET_CURRENT_CONTEXT(ctx);
>     struct gl_semaphore_object *semObj;
>     struct gl_buffer_object **bufObjs;
>     struct gl_texture_object **texObjs;
>
Ditto.

With the above nitpicks, patch is
Reviewed-by: Emil Velikov <emil.velikov@collabora.com>

-Emil
On 2018-02-01 08:24 AM, Emil Velikov wrote:
> On 31 January 2018 at 19:03, Andres Rodriguez <andresx7@gmail.com> wrote:
>> Don't want an overly large numBufferBarriers/numTextureBarriers to blow
>> up the stack.
>>
>> v2: handle malloc errors
>> v3: fix patch
>>
>> Suggested-by: Emil Velikov <emil.velikov@collabora.com>
>> Signed-off-by: Andres Rodriguez <andresx7@gmail.com>
>> ---
>>   src/mesa/main/externalobjects.c | 48 +++++++++++++++++++++++++++++++++++------
>>   1 file changed, 42 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/mesa/main/externalobjects.c b/src/mesa/main/externalobjects.c
>> index 463debd268..a28d6dba6f 100644
>> --- a/src/mesa/main/externalobjects.c
>> +++ b/src/mesa/main/externalobjects.c
>> @@ -713,91 +713,127 @@ _mesa_WaitSemaphoreEXT(GLuint semaphore,
>>                          const GLuint *buffers,
>>                          GLuint numTextureBarriers,
>>                          const GLuint *textures,
>>                          const GLenum *srcLayouts)
>>   {
>>      GET_CURRENT_CONTEXT(ctx);
>>      struct gl_semaphore_object *semObj;
>>      struct gl_buffer_object **bufObjs;
>>      struct gl_texture_object **texObjs;
> Initialize texObjs (might as well do bufObjs) since on bufObjs
> allocation failure we'll end up feeding junk to free().
> 
>> -   bufObjs = alloca(sizeof(struct gl_buffer_object **) * numBufferBarriers);
>> +   bufObjs = malloc(sizeof(struct gl_buffer_object **) * numBufferBarriers);
>> +   if (!bufObjs) {
>> +      _mesa_error(ctx, GL_OUT_OF_MEMORY, "%s(numBufferBarriers=%u)",
>> +                  func, numBufferBarriers);
>> +      goto end;
>> +   }
>> +
> 
>> +end:
>> +   free(bufObjs);
>> +   free(texObjs);
>>   }
>>
>>   void GLAPIENTRY
>>   _mesa_SignalSemaphoreEXT(GLuint semaphore,
>>                            GLuint numBufferBarriers,
>>                            const GLuint *buffers,
>>                            GLuint numTextureBarriers,
>>                            const GLuint *textures,
>>                            const GLenum *dstLayouts)
>>   {
>>      GET_CURRENT_CONTEXT(ctx);
>>      struct gl_semaphore_object *semObj;
>>      struct gl_buffer_object **bufObjs;
>>      struct gl_texture_object **texObjs;
>>
> Ditto.
> 
> With the above nitpicks, patch is
> Reviewed-by: Emil Velikov <emil.velikov@collabora.com>

Those aren't nits, those are very real bugs. And I should've double 
checked the patch better before sending it out!

Thanks for the reviews, fixed with your feedback and pushed.

Regards,
Andres

> 
> -Emil
>
On Wed, Jan 31, 2018 at 11:03 AM, Andres Rodriguez <andresx7@gmail.com> wrote:
> Don't want an overly large numBufferBarriers/numTextureBarriers to blow
> up the stack.
>
> v2: handle malloc errors
> v3: fix patch
>
> Suggested-by: Emil Velikov <emil.velikov@collabora.com>
> Signed-off-by: Andres Rodriguez <andresx7@gmail.com>
> ---
>  src/mesa/main/externalobjects.c | 48 +++++++++++++++++++++++++++++++++++------
>  1 file changed, 42 insertions(+), 6 deletions(-)
>
> diff --git a/src/mesa/main/externalobjects.c b/src/mesa/main/externalobjects.c
> index 463debd268..a28d6dba6f 100644
> --- a/src/mesa/main/externalobjects.c
> +++ b/src/mesa/main/externalobjects.c
> @@ -713,91 +713,127 @@ _mesa_WaitSemaphoreEXT(GLuint semaphore,
>                         const GLuint *buffers,
>                         GLuint numTextureBarriers,
>                         const GLuint *textures,
>                         const GLenum *srcLayouts)
>  {
>     GET_CURRENT_CONTEXT(ctx);
>     struct gl_semaphore_object *semObj;
>     struct gl_buffer_object **bufObjs;
>     struct gl_texture_object **texObjs;
>
> +   const char *func = "glWaitSemaphoreEXT";
> +
>     if (!ctx->Extensions.EXT_semaphore) {
> -      _mesa_error(ctx, GL_INVALID_OPERATION, "glWaitSemaphoreEXT(unsupported)");
> +      _mesa_error(ctx, GL_INVALID_OPERATION, "%s(unsupported)", func);
>        return;
>     }
>
>     ASSERT_OUTSIDE_BEGIN_END(ctx);
>
>     semObj = _mesa_lookup_semaphore_object(ctx, semaphore);
>     if (!semObj)
>        return;
>
>     FLUSH_VERTICES(ctx, 0);
>     FLUSH_CURRENT(ctx, 0);
>
> -   bufObjs = alloca(sizeof(struct gl_buffer_object **) * numBufferBarriers);
> +   bufObjs = malloc(sizeof(struct gl_buffer_object **) * numBufferBarriers);

Coverity is noting that this is subtly wrong:

>>>     Passing argument "8UL /* sizeof (struct gl_buffer_object **) */ * numBufferBarriers" to function "malloc" and then casting the return value to "struct gl_buffer_object **" is suspicious.  In this particular case "sizeof (struct gl_buffer_object **)" happens to be equal to "sizeof (struct gl_buffer_object *)", but this is not a portable assumption.
800        bufObjs = malloc(sizeof(struct gl_buffer_object **) *
numBufferBarriers);

Same thing applies later in the same file for texObjs.