[Mesa-dev] [PATCH glx/glxglvnd] Avoid overflow in 'last' variable of FindGLXFunction(...)

[Emil Velikov]

On 28 July 2016 at 14:58, Stefan Dirsch <sndirsch at suse.de> wrote:
> On Thu, Jul 14, 2016 at 05:20:55PM +0100, Emil Velikov wrote:
>> On 14 July 2016 at 15:23, Eric Engestrom <eric.engestrom at imgtec.com> wrote:
>> > On Thu, Jul 14, 2016 at 03:21:20PM +0200, Stefan Dirsch wrote:
>> >> This 'last' variable used in FindGLXFunction(...) may become negative,
>> >> but has been defined as unsigned int resulting in an overflow,
>> >> finally resulting in a segfault when accessing _glXDispatchTableStrings[...].
>> >> Fixed this by definining it as signed int. 'first' variable also needs to be
>> >> defined as signed int. Otherwise condition for while loop fails due to C
>> >> implicitly converting signed to unsigned values before comparison.
>> >
>> > Indeed, `last` can become negative is when the name searched for is
>> > alphabetically less than the first entry in the dispatch table.
>> > On the penultimate round, we would have `first = 0` and `last = 1`.
>> > Next iteration of the while loop, middle becomes 0, `strcmp() > 0`
>> > and last = middle - 1, ie. -1.
>> >
>> > The same issue exists on the other side (name searched is after last
>> > entry), but until DI_FUNCTION_COUNT reaches UINT_MAX this wouldn't
>> > wrap around.
>> >
>> > It's unlikely we'll ever have more than INT_MAX entries in the dispatch
>> > table, so I think this patch is OK. I tried to find a better fix, but
>> > adding checks before updating first and last feels too heavy.
>> >
>> Indeed, reaching {U,}INT_MAX is extremely unlikely, thus we can avoid
>> adding extra checks.
>>
>> > Reviewed-by: Eric Engestrom <eric.engestrom at imgtec.com>
>> >
>> I'll add the stable tag and push this in a few minutes (as the fresh
>> doze of coffee kicks in).
>
> Thanks a lot!
>
>> Stefan, I'll double-check about the issue mentioned in the cover
>> letter and let you know (and/or send patches).
>
> Didn't hear back from you. Are you still planning to look into this? Or does
> it just work for you and I messed something up on my side?
>
>From an extensive look there's a few things:
- I'm using $ export __GLX_VENDOR_LIBRARY_NAME=mesa since the xserver
GLVND patches are only in master.
Note: The vendor name is set only when using AIGLX, which thinking
about it sounds like a bug and we should set it when using GLX in
general.

- When the envvar is set the libGLX constructor preemptively sets up
the vendor. If not we'll observe a bug (considering we fix the other
bug below) where NULL func. ptr is returned.

- Due to a double "glXlglX" the functions are piped through mesa's
glXGetProcAddressARB, thus the drawable mapping call is never called
in the glXCreateGLXPixmapMESA case, leading to a memory leak and/or
other issues.

- On top of all that there is a silly think-o in the FindGLXFunction code.

I've got patches for the latter two and I'm working on patches for the
former two issues(?).

Thanks
Emil