Re: [PATCH xserver] Xext: dynamically allocate the PanoramiXDepths[j].vids array

[Peter Hutterer]

On Tue, Jul 17, 2018 at 10:12:55PM -0700, Keith Packard wrote:
> Peter Hutterer  writes:
> 
> > Control flow is:
> >PanoramiXMaybeAddDepth() allocates an array size 240 (pDepth->numVisuals)
> >PanoramiXMaybeAddVisual() finds up to 270 matches (pScreen->numVisuals)
> >and writes those into the previously allocated array.
> >
> > This caused invalid reads/writes followed by eventually a double-free abort.
> >
> > Reproduced with xorg-integration-tests server test
> > XineramaTest.ScreenCrossing/* (and a bunch of others).
> >
> > Signed-off-by: Peter Hutterer 
> 
> Reviewed-by: Keith Packard 
> 
> (I'd complain about the lack of NULL check, but the original code didn't
> bother either)

I suspect our overall behaviour where malloc fails is somewhere between
unpredictable and undefined. I don't think any of that code has ever been
tested and right now it probably just means we fall over somewhere else than
where it actually happened.

Thanks for the quick review, much appreciated.

To gitlab.freedesktop.org:xorg/xserver.git
   1c7f34e99..93cafb082  master -> master

Cheers,
   Peter
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] Xext: dynamically allocate the PanoramiXDepths[j].vids array

[Keith Packard]

Peter Hutterer  writes:

> Control flow is:
>PanoramiXMaybeAddDepth() allocates an array size 240 (pDepth->numVisuals)
>PanoramiXMaybeAddVisual() finds up to 270 matches (pScreen->numVisuals)
>and writes those into the previously allocated array.
>
> This caused invalid reads/writes followed by eventually a double-free abort.
>
> Reproduced with xorg-integration-tests server test
> XineramaTest.ScreenCrossing/* (and a bunch of others).
>
> Signed-off-by: Peter Hutterer 

Reviewed-by: Keith Packard 

(I'd complain about the lack of NULL check, but the original code didn't
bother either)

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xserver] Xext: dynamically allocate the PanoramiXDepths[j].vids array

[Peter Hutterer]

Control flow is:
   PanoramiXMaybeAddDepth() allocates an array size 240 (pDepth->numVisuals)
   PanoramiXMaybeAddVisual() finds up to 270 matches (pScreen->numVisuals)
   and writes those into the previously allocated array.

This caused invalid reads/writes followed by eventually a double-free abort.

Reproduced with xorg-integration-tests server test
XineramaTest.ScreenCrossing/* (and a bunch of others).

Signed-off-by: Peter Hutterer 
---
 Xext/panoramiX.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/Xext/panoramiX.c b/Xext/panoramiX.c
index 844ea49ce..bd9c45b03 100644
--- a/Xext/panoramiX.c
+++ b/Xext/panoramiX.c
@@ -751,11 +751,7 @@ PanoramiXMaybeAddDepth(DepthPtr pDepth)
PanoramiXNumDepths, sizeof(DepthRec));
 PanoramiXDepths[j].depth = pDepth->depth;
 PanoramiXDepths[j].numVids = 0;
-/* XXX suboptimal, should grow these dynamically */
-if (pDepth->numVids)
-PanoramiXDepths[j].vids = xallocarray(pDepth->numVids, 
sizeof(VisualID));
-else
-PanoramiXDepths[j].vids = NULL;
+PanoramiXDepths[j].vids = NULL;
 }
 
 static void
@@ -796,6 +792,9 @@ PanoramiXMaybeAddVisual(VisualPtr pVisual)
 
 for (k = 0; k < PanoramiXNumDepths; k++) {
 if (PanoramiXDepths[k].depth == pVisual->nplanes) {
+PanoramiXDepths[k].vids = reallocarray(PanoramiXDepths[k].vids,
+   PanoramiXDepths[k].numVids 
+ 1,
+   sizeof(VisualID));
 PanoramiXDepths[k].vids[PanoramiXDepths[k].numVids] = pVisual->vid;
 PanoramiXDepths[k].numVids++;
 break;
-- 
2.17.1

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel