[Mesa-dev] egl: return EGL_BAD_ALLOC if _eglConvertAttribsToInt fails

Submitted by Marek Olšák on May 26, 2015, 9:59 p.m.

Details

Message ID 1432677598-28064-1-git-send-email-maraeo@gmail.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Marek Olšák May 26, 2015, 9:59 p.m.
From: Marek Olšák <marek.olsak@amd.com>

This is a fix for the EGL 1.5 patch series.
---
 src/egl/main/eglapi.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
index 7afc091..3df4968 100644
--- a/src/egl/main/eglapi.c
+++ b/src/egl/main/eglapi.c
@@ -263,13 +263,14 @@  _eglConvertAttribsToInt(const EGLAttrib *attr_list)
       while (attr_list[size] != EGL_NONE)
          size += 2;
 
-      if (size) {
-         size += 1; /* add space for EGL_NONE */
-         int_attribs = malloc(size * sizeof(int_attribs[0]));
+      size += 1; /* add space for EGL_NONE */
 
-         for (i = 0; i < size; i++)
-            int_attribs[i] = attr_list[i];
-      }
+      int_attribs = malloc(size * sizeof(int_attribs[0]));
+      if (!int_attribs)
+         return NULL;
+
+      for (i = 0; i < size; i++)
+         int_attribs[i] = attr_list[i];
    }
    return int_attribs;
 }
@@ -332,6 +333,9 @@  eglGetPlatformDisplay(EGLenum platform, void *native_display,
    EGLDisplay display;
    EGLint *int_attribs = _eglConvertAttribsToInt(attrib_list);
 
+   if (attrib_list && !int_attribs)
+      RETURN_EGL_ERROR(NULL, EGL_BAD_ALLOC, NULL);
+
    display = eglGetPlatformDisplayEXT(platform, native_display, int_attribs);
    free(int_attribs);
    return display;
@@ -786,6 +790,9 @@  eglCreatePlatformWindowSurface(EGLDisplay dpy, EGLConfig config,
    EGLSurface surface;
    EGLint *int_attribs = _eglConvertAttribsToInt(attrib_list);
 
+   if (attrib_list && !int_attribs)
+      RETURN_EGL_ERROR(NULL, EGL_BAD_ALLOC, EGL_NO_SURFACE);
+
    surface = eglCreatePlatformWindowSurfaceEXT(dpy, config, native_window,
                                                int_attribs);
    free(int_attribs);
@@ -854,6 +861,9 @@  eglCreatePlatformPixmapSurface(EGLDisplay dpy, EGLConfig config,
    EGLSurface surface;
    EGLint *int_attribs = _eglConvertAttribsToInt(attrib_list);
 
+   if (attrib_list && !int_attribs)
+      RETURN_EGL_ERROR(NULL, EGL_BAD_ALLOC, EGL_NO_SURFACE);
+
    surface = eglCreatePlatformPixmapSurfaceEXT(dpy, config, native_pixmap,
                                                int_attribs);
    free(int_attribs);
@@ -1343,6 +1353,9 @@  eglCreateImage(EGLDisplay dpy, EGLContext ctx, EGLenum target,
    EGLImage image;
    EGLint *int_attribs = _eglConvertAttribsToInt(attr_list);
 
+   if (attr_list && !int_attribs)
+      RETURN_EGL_ERROR(NULL, EGL_BAD_ALLOC, EGL_NO_IMAGE);
+
    image = eglCreateImageKHR(dpy, ctx, target, buffer, int_attribs);
    free(int_attribs);
    return image;

Comments

On Wed, May 27, 2015 at 3:07 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> On 26/05/15 21:59, Marek Olšák wrote:
>> From: Marek Olšák <marek.olsak@amd.com>
>>
>> This is a fix for the EGL 1.5 patch series.
> Can you squash this with the offending patches (12 and 13). Pretty
> please :-)
>
>> ---
>>  src/egl/main/eglapi.c | 25 +++++++++++++++++++------
>>  1 file changed, 19 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
>> index 7afc091..3df4968 100644
>> --- a/src/egl/main/eglapi.c
>> +++ b/src/egl/main/eglapi.c
>> @@ -263,13 +263,14 @@ _eglConvertAttribsToInt(const EGLAttrib *attr_list)
>>        while (attr_list[size] != EGL_NONE)
>>           size += 2;
>>
>> -      if (size) {
>> -         size += 1; /* add space for EGL_NONE */
>> -         int_attribs = malloc(size * sizeof(int_attribs[0]));
>> +      size += 1; /* add space for EGL_NONE */
>>
>> -         for (i = 0; i < size; i++)
>> -            int_attribs[i] = attr_list[i];
>> -      }
>> +      int_attribs = malloc(size * sizeof(int_attribs[0]));
>> +      if (!int_attribs)
>> +         return NULL;
>> +
>> +      for (i = 0; i < size; i++)
>> +         int_attribs[i] = attr_list[i];
> Set the final value int_attrib[size] to EGL_NONE ?

EGL_NONE is already set by the user, that's why "size" is always +1.

Marek
On Wed, May 27, 2015 at 3:20 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> On 27/05/15 00:12, Marek Olšák wrote:
>> On Wed, May 27, 2015 at 3:07 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>> On 26/05/15 21:59, Marek Olšák wrote:
>>>> From: Marek Olšák <marek.olsak@amd.com>
>>>>
>>>> This is a fix for the EGL 1.5 patch series.
>>> Can you squash this with the offending patches (12 and 13). Pretty
>>> please :-)
>>>
>>>> ---
>>>>  src/egl/main/eglapi.c | 25 +++++++++++++++++++------
>>>>  1 file changed, 19 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
>>>> index 7afc091..3df4968 100644
>>>> --- a/src/egl/main/eglapi.c
>>>> +++ b/src/egl/main/eglapi.c
>>>> @@ -263,13 +263,14 @@ _eglConvertAttribsToInt(const EGLAttrib *attr_list)
>>>>        while (attr_list[size] != EGL_NONE)
>>>>           size += 2;
>>>>
>>>> -      if (size) {
>>>> -         size += 1; /* add space for EGL_NONE */
>>>> -         int_attribs = malloc(size * sizeof(int_attribs[0]));
>>>> +      size += 1; /* add space for EGL_NONE */
>>>>
>>>> -         for (i = 0; i < size; i++)
>>>> -            int_attribs[i] = attr_list[i];
>>>> -      }
>>>> +      int_attribs = malloc(size * sizeof(int_attribs[0]));
>>>> +      if (!int_attribs)
>>>> +         return NULL;
>>>> +
>>>> +      for (i = 0; i < size; i++)
>>>> +         int_attribs[i] = attr_list[i];
>>> Set the final value int_attrib[size] to EGL_NONE ?
>>
>> EGL_NONE is already set by the user, that's why "size" is always +1.
>>
> Most likely I'm missing something extremely obvious, so if you can bare
> with me that'll be appreciated:
>  - First we walk through the attr_list, looking for the size (without
> the sentinel/terminating EGL_NONE).
>  - Then we allocate a "size + 1" sized array and copy only the first
> "size" elements.

"size" is in the form "2*n+1" because of these:

while (attr_list[size] != EGL_NONE)
   size += 2;
size += 1;

Then, "size" elements are allocated and copied.

Marek
On 27 May 2015 at 01:40, Marek Olšák <maraeo@gmail.com> wrote:
> On Wed, May 27, 2015 at 3:20 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>> On 27/05/15 00:12, Marek Olšák wrote:
>>> On Wed, May 27, 2015 at 3:07 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>>> On 26/05/15 21:59, Marek Olšák wrote:
>>>>> From: Marek Olšák <marek.olsak@amd.com>
>>>>>
>>>>> This is a fix for the EGL 1.5 patch series.
>>>> Can you squash this with the offending patches (12 and 13). Pretty
>>>> please :-)
>>>>
>>>>> ---
>>>>>  src/egl/main/eglapi.c | 25 +++++++++++++++++++------
>>>>>  1 file changed, 19 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
>>>>> index 7afc091..3df4968 100644
>>>>> --- a/src/egl/main/eglapi.c
>>>>> +++ b/src/egl/main/eglapi.c
>>>>> @@ -263,13 +263,14 @@ _eglConvertAttribsToInt(const EGLAttrib *attr_list)
>>>>>        while (attr_list[size] != EGL_NONE)
>>>>>           size += 2;
>>>>>
>>>>> -      if (size) {
>>>>> -         size += 1; /* add space for EGL_NONE */
>>>>> -         int_attribs = malloc(size * sizeof(int_attribs[0]));
>>>>> +      size += 1; /* add space for EGL_NONE */
>>>>>
>>>>> -         for (i = 0; i < size; i++)
>>>>> -            int_attribs[i] = attr_list[i];
>>>>> -      }
>>>>> +      int_attribs = malloc(size * sizeof(int_attribs[0]));
>>>>> +      if (!int_attribs)
>>>>> +         return NULL;
>>>>> +
>>>>> +      for (i = 0; i < size; i++)
>>>>> +         int_attribs[i] = attr_list[i];
>>>> Set the final value int_attrib[size] to EGL_NONE ?
>>>
>>> EGL_NONE is already set by the user, that's why "size" is always +1.
>>>
>> Most likely I'm missing something extremely obvious, so if you can bare
>> with me that'll be appreciated:
>>  - First we walk through the attr_list, looking for the size (without
>> the sentinel/terminating EGL_NONE).
>>  - Then we allocate a "size + 1" sized array and copy only the first
>> "size" elements.
>
> "size" is in the form "2*n+1" because of these:
>
> while (attr_list[size] != EGL_NONE)
>    size += 2;
> size += 1;
>
> Then, "size" elements are allocated and copied.
>
I'm a goon. Looking at this multiple times and I've always parsed it
as "malloc(size + 1)" rather than "size++; malloc(size)".

Thanks for the patience
Emil
On 26/05/15 21:59, Marek Olšák wrote:
> From: Marek Olšák <marek.olsak@amd.com>
> 
> This is a fix for the EGL 1.5 patch series.
Can you squash this with the offending patches (12 and 13). Pretty
please :-)

> ---
>  src/egl/main/eglapi.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
> index 7afc091..3df4968 100644
> --- a/src/egl/main/eglapi.c
> +++ b/src/egl/main/eglapi.c
> @@ -263,13 +263,14 @@ _eglConvertAttribsToInt(const EGLAttrib *attr_list)
>        while (attr_list[size] != EGL_NONE)
>           size += 2;
>  
> -      if (size) {
> -         size += 1; /* add space for EGL_NONE */
> -         int_attribs = malloc(size * sizeof(int_attribs[0]));
> +      size += 1; /* add space for EGL_NONE */
>  
> -         for (i = 0; i < size; i++)
> -            int_attribs[i] = attr_list[i];
> -      }
> +      int_attribs = malloc(size * sizeof(int_attribs[0]));
> +      if (!int_attribs)
> +         return NULL;
> +
> +      for (i = 0; i < size; i++)
> +         int_attribs[i] = attr_list[i];
Set the final value int_attrib[size] to EGL_NONE ?

Thanks
Emil
On 27/05/15 00:12, Marek Olšák wrote:
> On Wed, May 27, 2015 at 3:07 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>> On 26/05/15 21:59, Marek Olšák wrote:
>>> From: Marek Olšák <marek.olsak@amd.com>
>>>
>>> This is a fix for the EGL 1.5 patch series.
>> Can you squash this with the offending patches (12 and 13). Pretty
>> please :-)
>>
>>> ---
>>>  src/egl/main/eglapi.c | 25 +++++++++++++++++++------
>>>  1 file changed, 19 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
>>> index 7afc091..3df4968 100644
>>> --- a/src/egl/main/eglapi.c
>>> +++ b/src/egl/main/eglapi.c
>>> @@ -263,13 +263,14 @@ _eglConvertAttribsToInt(const EGLAttrib *attr_list)
>>>        while (attr_list[size] != EGL_NONE)
>>>           size += 2;
>>>
>>> -      if (size) {
>>> -         size += 1; /* add space for EGL_NONE */
>>> -         int_attribs = malloc(size * sizeof(int_attribs[0]));
>>> +      size += 1; /* add space for EGL_NONE */
>>>
>>> -         for (i = 0; i < size; i++)
>>> -            int_attribs[i] = attr_list[i];
>>> -      }
>>> +      int_attribs = malloc(size * sizeof(int_attribs[0]));
>>> +      if (!int_attribs)
>>> +         return NULL;
>>> +
>>> +      for (i = 0; i < size; i++)
>>> +         int_attribs[i] = attr_list[i];
>> Set the final value int_attrib[size] to EGL_NONE ?
> 
> EGL_NONE is already set by the user, that's why "size" is always +1.
> 
Most likely I'm missing something extremely obvious, so if you can bare
with me that'll be appreciated:
 - First we walk through the attr_list, looking for the size (without
the sentinel/terminating EGL_NONE).
 - Then we allocate a "size + 1" sized array and copy only the first
"size" elements.
 - Thus the final element is garbage as we've used malloc().
 - Then we feed the (non-termintated) array into the respective function
and things go... funny.

Thanks
Emil