[Mesa-dev] [Mesa-stable] [PATCH] glx: fix crash with bad fbconfig

[Tapani Pälli]

On 05/31/2016 07:44 AM, Tapani Pälli wrote:
> Hi;
>
> On 05/30/2016 04:48 PM, Emil Velikov wrote:
>> Hi gents,
>>
>> On 30 May 2016 at 10:13, Tapani Pälli <tapani.palli at intel.com> wrote:
>>> From: Daniel Czarnowski <daniel.czarnowski at intel.com>
>>>
>>> GLX documentation states:
>>>         glXCreateNewContext can generate the following errors: (...)
>>>         GLXBadFBConfig if config is not a valid GLXFBConfig
>>>
>>> Function checks if the given config is a valid config and sets proper
>>> error code.
>>>
>>> Fixes currently crashing glx-fbconfig-bad Piglit test.
>>>
>>> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>>> Cc: "11.2" <mesa-stable at lists.freedesktop.org>
>>> ---
>>>  src/glx/glxcmds.c | 27 +++++++++++++++++++++++++++
>>>  1 file changed, 27 insertions(+)
>>>
>>> diff --git a/src/glx/glxcmds.c b/src/glx/glxcmds.c
>>> index bff01d2..4bc7fc4 100644
>>> --- a/src/glx/glxcmds.c
>>> +++ b/src/glx/glxcmds.c
>>> @@ -1629,8 +1629,35 @@ _X_EXPORT GLXContext
>>>  glXCreateNewContext(Display * dpy, GLXFBConfig fbconfig,
>>>                      int renderType, GLXContext shareList, Bool
>>> allowDirect)
>>>  {
>>> +   int list_size;
>>>     struct glx_config *config = (struct glx_config *) fbconfig;
>>>
>>> +   if (!config)
>>> +   {
>> Existing coding style is to put the opening bracket trailing on the
>> previous line. Same goes for the rest of the patch.
>
> oops, it seems I did not pay enough attention to style with this patch,
> got also some comments from Topi offline, will fix these, thanks Emil!
>
>>> +       __glXSendError(dpy, GLXBadFBConfig, 0, X_GLXCreateNewContext,
>>> false);
>>> +       return NULL;
>>> +   }
>>> +
>>> +   int screen = XDefaultScreen(dpy);
>> Use the DefaultScreen macro instead ?
>>
>>> +   struct glx_config **config_list = (struct glx_config **)
>>> +      glXGetFBConfigs(dpy, screen, &list_size);
>>> +
>> Worth checking (& bail) that list_size is negative ?

forgot to address this ...

glXGetFBConfigs may return NULL, however list_size is always at least 0 
so the current flow should be OK, in this case we will hit the error 
check where i == list_size.


>>
>>> +   int i;
>> unsigned i...
>>
>>> +   for (i = 0; i < list_size; i++)
>> ... and cast list_size to unsigned ?
>>
>>> +   {
>>> +       if (config_list[i] == config)
>>> +       {
>>> +           break;
>>> +       }
>>> +   }
>> Coding style: drop the brackets if there's only a single nested
>> statement. I.e. the above two pairs can go.
>>
>>> +   free(config_list);
>>> +
>>> +   if (i == list_size)
>>> +   {
>>> +       __glXSendError(dpy, GLXBadFBConfig, 0, X_GLXCreateNewContext,
>>> false);
>>> +       return NULL;
>>> +   }
>>> +
>>
>> In general, please don't mix variable declarations and code.
>>
>> Thanks
>> Emil
>>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev