Guaranteed Server crash with 4.4.0 (RC1)
Having looked at Bugzilla #978 it shows that it's very easy to crash the Xserver when using out-of-bounds coordinates that get mixed up when passing in int's that get converted to short's during the client->server conversation. Seeing as PutImage gets pushed through the CopyArea path, I'm sure the same problem can happen with the core protocol request for XCopyArea() too (and possibly others). There's obvious ways to fix this, but I'm keen to hear others views... Alan. ___ Devel mailing list [EMAIL PROTECTED] http://XFree86.Org/mailman/listinfo/devel
Re: Guaranteed Server crash with 4.4.0 (RC1)
I don't think it's as bad as you think. It looks to me like this comes about due to a difference in the Shm protocol. Going against convention, xShmPutImageReq has an unsigned value for the src X and Y location. All other primitive have signed values. I think the correct behavior is probably to clamp the source X and Y in ProcShmPutImage function to the unsigned 15 bit coordinate system. Can somebody try that to see if it fixes the problem? Mark. On Wed, 17 Dec 2003, Alan Hourihane wrote: > Having looked at Bugzilla #978 it shows that it's very easy to crash > the Xserver when using out-of-bounds coordinates that get mixed up when > passing in int's that get converted to short's during the client->server > conversation. > > Seeing as PutImage gets pushed through the CopyArea path, I'm sure > the same problem can happen with the core protocol request for XCopyArea() > too (and possibly others). > > There's obvious ways to fix this, but I'm keen to hear others views... > > Alan. > ___ > Devel mailing list > [EMAIL PROTECTED] > http://XFree86.Org/mailman/listinfo/devel > ___ Devel mailing list [EMAIL PROTECTED] http://XFree86.Org/mailman/listinfo/devel
Re: Guaranteed Server crash with 4.4.0 (RC1)
Ah yes. I skimmed over shmstr.h too quickly and assumed INT16 instead of CARD16 for the source coords. I'll try this now. Alan. On Wed, Dec 17, 2003 at 03:08:03PM -0800, Mark Vojkovich wrote: >I don't think it's as bad as you think. It looks to me like > this comes about due to a difference in the Shm protocol. Going > against convention, xShmPutImageReq has an unsigned value for the > src X and Y location. All other primitive have signed values. > I think the correct behavior is probably to clamp the source X and > Y in ProcShmPutImage function to the unsigned 15 bit coordinate > system. Can somebody try that to see if it fixes the problem? > > Mark. > > > On Wed, 17 Dec 2003, Alan Hourihane wrote: > > > Having looked at Bugzilla #978 it shows that it's very easy to crash > > the Xserver when using out-of-bounds coordinates that get mixed up when > > passing in int's that get converted to short's during the client->server > > conversation. > > > > Seeing as PutImage gets pushed through the CopyArea path, I'm sure > > the same problem can happen with the core protocol request for XCopyArea() > > too (and possibly others). > > > > There's obvious ways to fix this, but I'm keen to hear others views... > > > > Alan. > > ___ > > Devel mailing list > > [EMAIL PROTECTED] > > http://XFree86.Org/mailman/listinfo/devel > > > > ___ > Devel mailing list > [EMAIL PROTECTED] > http://XFree86.Org/mailman/listinfo/devel ___ Devel mailing list [EMAIL PROTECTED] http://XFree86.Org/mailman/listinfo/devel
Re: Guaranteed Server crash with 4.4.0 (RC1)
On Wed, Dec 17, 2003 at 03:08:03PM -0800, Mark Vojkovich wrote: >I don't think it's as bad as you think. It looks to me like > this comes about due to a difference in the Shm protocol. Going > against convention, xShmPutImageReq has an unsigned value for the > src X and Y location. All other primitive have signed values. > I think the correct behavior is probably to clamp the source X and > Y in ProcShmPutImage function to the unsigned 15 bit coordinate > system. Can somebody try that to see if it fixes the problem? Yup. This fixes it. Just committed. Alan. Index: shm.c === RCS file: /X11R6/x-cvs/xc/programs/Xserver/Xext/shm.c,v retrieving revision 3.40 diff -u -r3.40 shm.c --- shm.c 17 Nov 2003 22:20:27 - 3.40 +++ shm.c 17 Dec 2003 23:20:06 - @@ -815,6 +815,8 @@ REQUEST_SIZE_MATCH(xShmPutImageReq); VALIDATE_DRAWABLE_AND_GC(stuff->drawable, pDraw, pGC, client); VERIFY_SHMPTR(stuff->shmseg, stuff->offset, FALSE, shmdesc, client); +if (stuff->srcX > 32767 || stuff->srcY > 32767) + return BadValue; if ((stuff->sendEvent != xTrue) && (stuff->sendEvent != xFalse)) return BadValue; if (stuff->format == XYBitmap) ___ Devel mailing list [EMAIL PROTECTED] http://XFree86.Org/mailman/listinfo/devel
Re: Guaranteed Server crash with 4.4.0 (RC1)
On Wed, Dec 17, 2003 at 03:08:03PM -0800, Mark Vojkovich wrote: >I don't think it's as bad as you think. It looks to me like > this comes about due to a difference in the Shm protocol. Going > against convention, xShmPutImageReq has an unsigned value for the > src X and Y location. All other primitive have signed values. > I think the correct behavior is probably to clamp the source X and > Y in ProcShmPutImage function to the unsigned 15 bit coordinate > system. Can somebody try that to see if it fixes the problem? Actually, it doesn't fix it. And I've backed out that patch. Stephen uploaded another test app which shows that even with srcX/Y as 0 and a negative dstX/Y parameter can still crash the server. It looks like a bug in the way fb uses deltas whereas I've tested the original cfb code and it works fine. Alan. ___ Devel mailing list [EMAIL PROTECTED] http://XFree86.Org/mailman/listinfo/devel
Re: Guaranteed Server crash with 4.4.0 (RC1)
Looking at ProcShmGetImage() there's a bunch of checking for out-of-bounds
coordinates, but ProcShmPutImage() lacks this checking.
Is this patch reasonable or too much (it does fix the problem) but I'm
wondering if the bounds are too strict for PutImage ?
Alan.
Index: shm.c
===
RCS file: /X11R6/x-cvs/xc/programs/Xserver/Xext/shm.c,v
retrieving revision 3.40
diff -u -r3.40 shm.c
--- shm.c 17 Nov 2003 22:20:27 - 3.40
+++ shm.c 18 Dec 2003 14:17:07 -
@@ -815,6 +815,34 @@
REQUEST_SIZE_MATCH(xShmPutImageReq);
VALIDATE_DRAWABLE_AND_GC(stuff->drawable, pDraw, pGC, client);
VERIFY_SHMPTR(stuff->shmseg, stuff->offset, FALSE, shmdesc, client);
+if (pDraw->type == DRAWABLE_WINDOW)
+{
+ if( /* check for being viewable */
+!((WindowPtr) pDraw)->realized ||
+ /* check for being on screen */
+ pDraw->x + stuff->dstX < 0 ||
+pDraw->x + stuff->dstX + (int)stuff->srcWidth > pDraw->pScreen->width ||
+ pDraw->y + stuff->dstY < 0 ||
+ pDraw->y + stuff->dstY + (int)stuff->srcHeight > pDraw->pScreen->height ||
+ /* check for being inside of border */
+ stuff->dstX < - wBorderWidth((WindowPtr)pDraw) ||
+ stuff->dstX + (int)stuff->srcWidth >
+ wBorderWidth((WindowPtr)pDraw) + (int)pDraw->width ||
+ stuff->dstY < -wBorderWidth((WindowPtr)pDraw) ||
+ stuff->dstY + (int)stuff->srcHeight >
+ wBorderWidth((WindowPtr)pDraw) + (int)pDraw->height
+)
+ return(BadMatch);
+}
+else
+{
+ if (stuff->dstX < 0 ||
+ stuff->dstX+(int)stuff->srcWidth > pDraw->width ||
+ stuff->dstY < 0 ||
+ stuff->dstY+(int)stuff->srcHeight > pDraw->height
+ )
+ return(BadMatch);
+}
if ((stuff->sendEvent != xTrue) && (stuff->sendEvent != xFalse))
return BadValue;
if (stuff->format == XYBitmap)
___
Devel mailing list
[EMAIL PROTECTED]
http://XFree86.Org/mailman/listinfo/devel
Re: Guaranteed Server crash with 4.4.0 (RC1)
Around 14 o'clock on Dec 18, Alan Hourihane wrote: > Is this patch reasonable or too much (it does fix the problem) but I'm > wondering if the bounds are too strict for PutImage ? Preserved window contents may not be limited to screen geometry. ___ Devel mailing list [EMAIL PROTECTED] http://XFree86.Org/mailman/listinfo/devel
Re: Guaranteed Server crash with 4.4.0 (RC1)
On Thu, Dec 18, 2003 at 09:56:01AM -0800, Keith Packard wrote: > Around 14 o'clock on Dec 18, Alan Hourihane wrote: > > > Is this patch reasonable or too much (it does fix the problem) but I'm > > wondering if the bounds are too strict for PutImage ? > > Preserved window contents may not be limited to screen geometry. It's looking more like a bug in fb then, as cfb doesn't exhibit this problem. Alan. ___ Devel mailing list [EMAIL PROTECTED] http://XFree86.Org/mailman/listinfo/devel
Re: Guaranteed Server crash with 4.4.0 (RC1)
I believe that's wrong behavior. GetImage is spec'd that way.
PutImage is not.
Mark.
On Thu, 18 Dec 2003, Alan Hourihane wrote:
> Looking at ProcShmGetImage() there's a bunch of checking for out-of-bounds
> coordinates, but ProcShmPutImage() lacks this checking.
>
> Is this patch reasonable or too much (it does fix the problem) but I'm
> wondering if the bounds are too strict for PutImage ?
>
> Alan.
>
> Index: shm.c
> ===
> RCS file: /X11R6/x-cvs/xc/programs/Xserver/Xext/shm.c,v
> retrieving revision 3.40
> diff -u -r3.40 shm.c
> --- shm.c 17 Nov 2003 22:20:27 - 3.40
> +++ shm.c 18 Dec 2003 14:17:07 -
> @@ -815,6 +815,34 @@
> REQUEST_SIZE_MATCH(xShmPutImageReq);
> VALIDATE_DRAWABLE_AND_GC(stuff->drawable, pDraw, pGC, client);
> VERIFY_SHMPTR(stuff->shmseg, stuff->offset, FALSE, shmdesc, client);
> +if (pDraw->type == DRAWABLE_WINDOW)
> +{
> + if( /* check for being viewable */
> + !((WindowPtr) pDraw)->realized ||
> + /* check for being on screen */
> + pDraw->x + stuff->dstX < 0 ||
> + pDraw->x + stuff->dstX + (int)stuff->srcWidth > pDraw->pScreen->width ||
> + pDraw->y + stuff->dstY < 0 ||
> + pDraw->y + stuff->dstY + (int)stuff->srcHeight > pDraw->pScreen->height ||
> + /* check for being inside of border */
> + stuff->dstX < - wBorderWidth((WindowPtr)pDraw) ||
> + stuff->dstX + (int)stuff->srcWidth >
> + wBorderWidth((WindowPtr)pDraw) + (int)pDraw->width ||
> + stuff->dstY < -wBorderWidth((WindowPtr)pDraw) ||
> + stuff->dstY + (int)stuff->srcHeight >
> + wBorderWidth((WindowPtr)pDraw) + (int)pDraw->height
> +)
> + return(BadMatch);
> +}
> +else
> +{
> + if (stuff->dstX < 0 ||
> + stuff->dstX+(int)stuff->srcWidth > pDraw->width ||
> + stuff->dstY < 0 ||
> + stuff->dstY+(int)stuff->srcHeight > pDraw->height
> + )
> + return(BadMatch);
> +}
> if ((stuff->sendEvent != xTrue) && (stuff->sendEvent != xFalse))
> return BadValue;
> if (stuff->format == XYBitmap)
> ___
> Devel mailing list
> [EMAIL PROTECTED]
> http://XFree86.Org/mailman/listinfo/devel
>
___
Devel mailing list
[EMAIL PROTECTED]
http://XFree86.Org/mailman/listinfo/devel
