Bug 18445

Nice find.  The code in question has been as-is for many years so this goes way back.

The problem is when the GLX reply is an array, the number of bytes returned is always a multiple of four.  If the 'residences' array isn't a multiple of four in size, out-of-bounds writes could occur.

The solution is to allocate a temporary buffer which is a multiple of four bytes when necessary.

I'm about to commit a fix to Mesa for this.  Unfortunately, it involves special-casing the indirect AreTextureResident() function whereas it was auto-generated before.

BTW, I suggest that you code around this problem in your application.  This bug is probably in every libGL in existance and it'll be a while before this fix is propagated.

Specifically, always use a GLboolean 'residences' array that's a multiple of four in size.

(In reply to comment #1)
> Nice find.  The code in question has been as-is for many years so this goes way
> back.
> 
> The problem is when the GLX reply is an array, the number of bytes returned is
> always a multiple of four.  If the 'residences' array isn't a multiple of four
> in size, out-of-bounds writes could occur.
> 
> The solution is to allocate a temporary buffer which is a multiple of four
> bytes when necessary.
> 
> I'm about to commit a fix to Mesa for this.  Unfortunately, it involves
> special-casing the indirect AreTextureResident() function whereas it was
> auto-generated before.
> 


I think I disagree here. I think that the real/main problem here is
that if __glXReadReply is given the  the last argument "true",
it will try to read the reply data using size 4 * reply.length
                                             ^^^
(indirect.c line 77). So using your patch wouldn't this cause 
an overrun of the malloc:ed buffer. (Or does _XRead limit this
based on available data or something).

I would actually suggest fixing this problem by using the the original
code, but just changing the last argument to GL_FALSE for __glXReadReply.
(I don't really know if this is ok, but it does not crash, and
similar code exists for example in glAreProgramsResidentNV )

(I have done my tests using the Mesa-7.2 source code,
Need to find out where dri2tokens.h is before I can compile
the git version....)

I am taking the liberty to marking this with "Reopen bug", 



           Eero

(In reply to comment #3)
> (In reply to comment #1)
 
> I am taking the liberty to marking this with "Reopen bug", 
> 
Maybe this time...

   Eero

Unfortunately, changing reply_is_always_array from GL_TRUE to GL_FALSE doesn't give the right results either.  I tried that yesterday.

If you write a test program that calls glAreTexturesResident() with n=3, then set a breakpoint in __glXReadReply() you'll see that bytes=1 and extra=3.

That's because reply looks like this:

{type = 1 '\001', unused = 0 '\0', sequenceNumber = 43, length = 1, retval = 0, 
  size = 1, pad3 = 16, pad4 = 0, pad5 = 0, pad6 = 0} and size=1.  So bytes = 1*1.

So, changing reply_is_always_array to GL_TRUE will work if n=1, but will not work for n>1.

Err, replace GL_TRUE w/ GL_FALSE in my last sentence there.

(In reply to comment #5)
> Unfortunately, changing reply_is_always_array from GL_TRUE to GL_FALSE doesn't
> give the right results either.  I tried that yesterday.
> 
> If you write a test program that calls glAreTexturesResident() with n=3, then
> set a breakpoint in __glXReadReply() you'll see that bytes=1 and extra=3.
> 
> That's because reply looks like this:
> 
> {type = 1 '\001', unused = 0 '\0', sequenceNumber = 43, length = 1, retval = 0, 
>   size = 1, pad3 = 16, pad4 = 0, pad5 = 0, pad6 = 0} and size=1.  So bytes =
> 1*1.
> 

I am getting even more lost..
(gdb) up
#1  0xb7eac675 in __indirect_glAreTexturesResident (n=3, textures=0xbfff5dfc, 
    residences=0xbfff5e00 "") at indirect.c:5112
(gdb) down
#0  __glXReadReply (dpy=0xddb3bd8, size=1, dest=0xbfff5e00, 
    reply_is_always_array=0 '\0') at indirect.c:81
(gdb) p bytes
$7 = 3
(gdb) p extra
$8 = 1
(gdb) p reply
$9 = {type = 1 '\001', unused = 0 '\0', sequenceNumber = 47906, length = 1, 
  retval = 0, size = 3, pad3 = 143228600, pad4 = 0, pad5 = 0, pad6 = 0}


This is on Mesa-7.2 released code, with the reply_is_always_array change
Mesa built for linux-dri-debug

  Eero

In any case our results agree on the reply.length field,
and that is the field used by the code path of your fix.

So I won't complain about it any more :-)


  Eero

Mass version move, cvs -> git