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

[Tapani Pälli]

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 ?
>
>> +   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
>