[Mesa-dev] [Mesa-stable] [PATCH 4/4] glx: fix error code when there is no context bound

[Tapani Pälli]

On 05/26/2016 05:16 PM, Emil Velikov wrote:
> Hi all,
>
> On 29 February 2016 at 07:14, Tapani Pälli <tapani.palli at intel.com> wrote:
>>
>> On 02/22/2016 10:16 PM, Ian Romanick wrote:
>>>
>>> There are 17 total occurrences of
>>>
>>>      grep -r '[(]!gc[)]' src/glx/
>>>
>>> and
>>>
>>>      grep -r 'gc[[:space:]]*==[[:space:]]*NULL' src/glx/
>>>
>>> None of these check for dummyContext.  This is all very suspicious.
>>> Looking at the implementation(s) of __glXGetCurrentContext, I don't
>>> think it can ever return NULL.  Look in src/glx/glxcurrent.c.  It's
>>> possible that __glXGetCurrentContext used to be able to return NULL, but
>>> I find it unlikely.
>>>
>>> My guess is that all (or nearly all) of the !gc or gc == NULL checks are
>>> wrong.  A bunch of them probably "just work" because they end up sending
>>> protocol requests to the server, and the server sends back an error.
>>
>>
>> I spent some time with this and it looks like some of these are correct as
>> create_context (or indirect_create_context) can return NULL and also pointer
>> given by client may be NULL (and can't be dummyContext). The places with
>> explicit __glXGetCurrentContext call (9 of these) and a NULL check are
>> incorrect. I can add these to the patch.
>>
>>> At the very least, I think these gc == NULL checks should be replaced by
>>> asserts.  If the unit tests call these functions with
>>> __glXGetCurrentContext returning NULL, the unit tests should be fixed to
>>> return &dummyContext instead.
>>
>>
>> Should it be then 'own dummyContext' implemented by fake_glx_screen.cpp
>> something along lines in this patch and not trying to link with
>> glxcurrent.c?
>>
>>> I'd really like to see analysis of the other NULL checks and either have
>>> justifications for no change or have changes.  I'd also really like to
>>> see piglit tests that could hit some of these.
>>
>>
>> It looks like glx-test is testing return value of __glXGetCurrentContext
>> currently (which is why it breaks), wouldn't fixing glx-test be sufficient?
>>
>>
> Any news on the status of this patch ? The Suse guys did bring some
> fixes recently (check __glXGetCurrentContext() vs dummyContext as
> opposed to NULL), although I think we still want something like the
> proposed here. Correct ?
>

No progress, this patch has been living as is in internal project. The 
fix itself is quite simple, all places with __glXGetCurrentContext 
should check against dummyContext.

This patch introduced its own 'dummyContext' in the unit test since it 
seemed very challenging to compile the test together with files in glx 
folder (results in linking with a *lot* of stuff). I can take a peek 
again what was the issue in replacing all of the checks and reply back 
to this.

// Tapani