[Mesa-dev] glxinfo proposed change

[Ian Romanick]

On 01/07/2013 01:08 PM, Jorge Ramirez Ortiz, HCL Europe wrote:
>
>
>> -----Original Message-----
>> From: Ian Romanick [mailto:idr at freedesktop.org]
>> Sent: Monday, January 07, 2013 7:48 PM
>> To: Jorge Ramirez Ortiz, HCL Europe
>> Cc: Dan Nicholson; mesa-dev at lists.freedesktop.org
>> Subject: Re: [Mesa-dev] glxinfo proposed change
>>
>> On 01/07/2013 06:35 AM, Jorge Ramirez Ortiz, HCL Europe wrote:
>>>> -----Original Message-----
>>>> From: Dan Nicholson [mailto:dbn.lists at gmail.com]
>>>> Sent: Monday, January 07, 2013 2:31 PM
>>>> To: Jorge Ramirez Ortiz, HCL Europe
>>>> Cc: mesa-dev at lists.freedesktop.org
>>>> Subject: Re: [Mesa-dev] glxinfo proposed change
>>>>
>>>> On Mon, Jan 7, 2013 at 5:06 AM, Jorge Ramirez Ortiz,  HCL Europe
>>>> <Jorge.Ramirez-Ortiz at hcl.com> wrote:
>>>>> Hi all,
>>>>>
>>>>> Does this patch make sense? glVersion can be NULL and not having the
>>>> changes below could cause a SIGSEGV
>>>>>
>>>>> [jramirez at calypso-2 mesa-demos.git (tmp *)]$ git diff src/xdemos/glxinfo.c
>>>>> diff --git a/src/xdemos/glxinfo.c b/src/xdemos/glxinfo.c
>>>>> index aa6430d..97b6539 100644
>>>>> --- a/src/xdemos/glxinfo.c
>>>>> +++ b/src/xdemos/glxinfo.c
>>>>> @@ -639,7 +639,7 @@ print_screen_info(Display *dpy, int scrnum, Bool
>>>> allowDirect, Bool limits, Bool
>>>>>          printf("OpenGL renderer string: %s\n", glRenderer);
>>>>>          printf("OpenGL version string: %s\n", glVersion);
>>>>>    #ifdef GL_VERSION_2_0
>>>>> -      if (glVersion[0] >= '2' && glVersion[1] == '.') {
>>>>> +      if (glVersion && glVersion[0] >= '2' && glVersion[1] == '.') {
>>>>>             char *v = (char *) glGetString(GL_SHADING_LANGUAGE_VERSION);
>>>>>             printf("OpenGL shading language version string: %s\n", v);
>>>>>          }
>>>>
>>>> Normally, this is a good idea. However, glVersion is already
>>>> dereferenced in the printf on the previous line. So, any actual
>>>> checking for NULL glVersion should at least come earlier in the
>>>> function. On the other hand, returning NULL from
>>>> glGetString(GL_VERSION) might be so rare that bothering to guard for
>>>> it would be more effort than it's worth.
>>>
>>> Bear in mind that printf of a null string wont SIGSEGV.
>>> Only trying to access glVersion[] elements presents -and indeed is - a risk and
>> therefore a critical bug (for my usual standards that is).
>>
>> The critical bug is that glGetString is not allowed to return NULL.
>> Your GL is horrifically broken in that case, and no application will
>> work either.
>
> You are absolutely wrong. NULL is a valid return value. Please read the code.

I've been working on this project for almost twelve years, and I've been 
a voting member of the OpenGL Architecture Review Board for almost as 
long.  I'm quite familiar with the code.

glGetString returns 0 only when an error is generated, and it is not 
allowed to generate an error for a valid enum, such as GL_VERSION, when 
there is a context.  Previous code in glxinfo has already made the 
context current and checked that it was successful.

I wasn't interested in seeing this patch ever land, and after your rude, 
disrespectful attitude, I'm even less interested.