[PULL] Bugzilla triage, take two

[Adam Jackson]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <http://lists.x.org/archives/xorg-devel/attachments/20100326/267fc39c/attachment.pgp>