[PULL] Bugzilla triage, take two

[Corbin Simpson]

On Fri, Mar 26, 2010 at 11:43 AM, Adam Jackson <ajax at nwnk.net> wrote:
> On Fri, 2010-03-26 at 06:54 -0700, Corbin Simpson wrote:
>
>>   Rami Ylimaki (1):
>>         os: Prevent backtrace from being stopped in noreturn functions.
>>
>> are available in the git repository at:
>>
>>   git://anongit.freedesktop.org/~csimpson/xserver triage
>>
>> Corbin Simpson (1):
>>       dix: Fix a small potential memory leak in ProcRotateProperties. (#25216)
>
> Ack.
>
>> David Jander (1):
>>       kdrive: Fix incomplete rotation matrix. (#20941)
>
> Looks plausible, although I don't really understand the rotation code.
>
>> Evgeny M. Zubok (1):
>>       xfree86: Change VBE version early-out to 1.2. (#22672)
>
> Ack, although there's no real reason for that version check at all.  If
> the VBE call isn't present we'll fail no matter what.
>
>> Ilpo Ruotsalainen (1):
>>       shadow: Optimize shadowUpdatePacked(). (#26973)
>
> Nack.  Read the code (abridged):
>
>>         while (h--)
>>         {
>> // ...
>>             sha = shaLine;
>>             while (width) {
>> // ...
>>                 win = winBase + (scr - scrBase);
>> // ...
>>                 while (i--)
>>                     *win++ = *sha++;
>>             }
>>             shaLine += shaStride;
>>             y++;
>>         }
>
> The proposed replacement for that while (i--) is
>
>    memcpy(win, sha, i * sizeof(FbBits));
>
> But that's not equivalent; in the memcpy version, sha never moves.  So
> if you ever go around the while (width) loop more than once, you'll be
> moving the destination of the memcpy, but not the source.  I think you
> want:
>
>    memcpy(win, sha, i * sizeof(FbBits));
>    sha += i;
>
>> Joe Rabinoff (1):
>>       xfree86: Fix off-by-one math in screen rotation. (#20762)
>
> Again, looks plausible.
>
>> TSUKAHARA Ken (1):
>>       dix: Prevent denial-of-color attack. (#9356)
>
> Nack in its current form at least.  These hunks are bogus:
>
>> --- a/dix/colormap.c
>> +++ b/dix/colormap.c
>> @@ -811,6 +811,7 @@ AllocColor (ColormapPtr pmap,
>>      VisualPtr        pVisual;
>>      int              npix;
>>      Pixel    *ppix;
>> +    int         initialPixels = pmap->numPixelsRed[client];
>>
>>      pVisual = pmap->pVisual;
>>      (*pmap->pScreen->ResolveColor) (pred, pgreen, pblue, pVisual);
>> @@ -970,8 +971,15 @@ AllocColor (ColormapPtr pmap,
>>       }
>>       pcr->mid = pmap->mid;
>>       pcr->client = client;
>> -     if (!AddResource(FakeClientID(client), RT_CMAPENTRY, (pointer)pcr))
>> -         return (BadAlloc);
>> +        if (class == PseudoColor) {
>> +            if (initialPixels == 0) {
>> +             if (!AddResource(FakeClientID(client), RT_CMAPENTRY, (pointer)pcr))
>> +                 return (BadAlloc);
>> +            }
>> +        } else {
>> +         if (!AddResource(FakeClientID(client), RT_CMAPENTRY, (pointer)pcr))
>> +             return (BadAlloc);
>> +        }
>>      }
>>      return (Success);
>>  }
>
> In the second hunk, we're inside a conditional on
>
>    if ((pmap->numPixelsRed[client] == 1) && /* ... */
>
> And, if we're PseudoColor, we don't increment numPixelsRed[client]
> between these two hunks.  So the if (initialPixels == 0) conditional is
> really if (0).  Which is definitely wrong; if a client allocates a color
> in a pseudocolor map, that color needs to be GC'd at client death.
>
> The rest of it seems to be changing ->refcnt in EntryPtr from "count of
> how many times this color has been allocated" to "count of how many
> clients have allocated this color", which I'm pretty sure would need to
> be reflected in more places than just FindColor.
>
> - ajax
>

Alrighty. I've closed the denial-of-color bug based on your comments
and the general age and silliness of the code, and dropped that patch.
I went ahead and split the branch into bugfixes and triage; please
only pull from bugfixes.

~ C.

The following changes since commit 579715f830fbbca9e1ecb17dc18176132f5969e7:
  Rami Ylimaki (1):
        os: Prevent backtrace from being stopped in noreturn functions.

are available in the git repository at:

  git://anongit.freedesktop.org/~csimpson/xserver bugfixes

Corbin Simpson (1):
      dix: Fix a small potential memory leak in ProcRotateProperties. (#25216)

Evgeny M. Zubok (1):
      xfree86: Change VBE version early-out to 1.2. (#22672)

 dix/property.c       |    6 ++++--
 hw/xfree86/vbe/vbe.c |    2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

-- 
When the facts change, I change my mind. What do you do, sir? ~ Keynes

Corbin Simpson
<MostAwesomeDude at gmail.com>