On Thu, 12 Dec 2002 22:26:23 +0000
Keith Whitwell <[EMAIL PROTECTED]> wrote:

> Felix Kühling wrote:
> > Hi,
> > 
> > while I was messing around with my query programme I found this: if I
> > specify an invalid screen as argument to XF86DRIGetClientDriverName the
> > Xserver segfaults. I had a quick look at xc/xc/Xserver/GL/dri/xf86dri.c.
> > stuff->screen is used as array index without checking. I'm not sure
> > though, where would be the right place to fix it.
> > 
> > Other functions in xf86dri.c must be affacted, too. They use
> > stuff->screen in the same way.
> 
> Those functions should validate any information they receive over the wire, as 
> soon as is feasible.

The problem is that the request records are all different. So we can't
check stuff->screen in ProcXF86DRIDispatch. We have to do it in each
request separately.

The attached patch does just that. While I grepped around to see how
other extensions check this I found one more potential problem in
programs/Xserver/GL/glx/glxcmds.c and two suspicious casts of screen
from unsigned to int (screen is always unsigned in the request records).
They are fixed in the attached patch as well.

Regards,
   Felix

               __\|/__    ___     ___     ___
__Tschüß_______\_6 6_/___/__ \___/__ \___/___\___You can do anything,___
_____Felix_______\Ä/\ \_____\ \_____\ \______U___just not everything____
  [EMAIL PROTECTED]    >o<__/   \___/   \___/        at the same time!



Index: dri/xf86dri.c
===================================================================
RCS file: /cvsroot/dri/xc/xc/programs/Xserver/GL/dri/xf86dri.c,v
retrieving revision 1.10
diff -u -r1.10 xf86dri.c
--- dri/xf86dri.c       29 Oct 2002 20:28:57 -0000      1.10
+++ dri/xf86dri.c       13 Dec 2002 15:51:57 -0000
@@ -155,6 +155,11 @@
 
     REQUEST(xXF86DRIQueryDirectRenderingCapableReq);
     REQUEST_SIZE_MATCH(xXF86DRIQueryDirectRenderingCapableReq);
+    if (stuff->screen >= screenInfo.numScreens) {
+       client->errorValue = stuff->screen;
+       return BadValue;
+    }
+
     rep.type = X_Reply;
     rep.length = 0;
     rep.sequenceNumber = client->sequence;
@@ -184,6 +189,10 @@
 
     REQUEST(xXF86DRIOpenConnectionReq);
     REQUEST_SIZE_MATCH(xXF86DRIOpenConnectionReq);
+    if (stuff->screen >= screenInfo.numScreens) {
+       client->errorValue = stuff->screen;
+       return BadValue;
+    }
 
     if (!DRIOpenConnection( screenInfo.screens[stuff->screen], 
                            &hSAREA,
@@ -221,6 +230,10 @@
     
     REQUEST(xXF86DRIAuthConnectionReq);
     REQUEST_SIZE_MATCH(xXF86DRIAuthConnectionReq);
+    if (stuff->screen >= screenInfo.numScreens) {
+       client->errorValue = stuff->screen;
+       return BadValue;
+    }
 
     rep.type = X_Reply;
     rep.length = 0;
@@ -242,6 +255,10 @@
 {
     REQUEST(xXF86DRICloseConnectionReq);
     REQUEST_SIZE_MATCH(xXF86DRICloseConnectionReq);
+    if (stuff->screen >= screenInfo.numScreens) {
+       client->errorValue = stuff->screen;
+       return BadValue;
+    }
 
     DRICloseConnection( screenInfo.screens[stuff->screen]);
 
@@ -258,6 +275,10 @@
 
     REQUEST(xXF86DRIGetClientDriverNameReq);
     REQUEST_SIZE_MATCH(xXF86DRIGetClientDriverNameReq);
+    if (stuff->screen >= screenInfo.numScreens) {
+       client->errorValue = stuff->screen;
+       return BadValue;
+    }
 
     DRIGetClientDriverName( screenInfo.screens[stuff->screen],
                            (int *)&rep.ddxDriverMajorVersion,
@@ -295,6 +316,11 @@
 
     REQUEST(xXF86DRICreateContextReq);
     REQUEST_SIZE_MATCH(xXF86DRICreateContextReq);
+    if (stuff->screen >= screenInfo.numScreens) {
+       client->errorValue = stuff->screen;
+       return BadValue;
+    }
+
     rep.type = X_Reply;
     rep.length = 0;
     rep.sequenceNumber = client->sequence;
@@ -329,6 +355,10 @@
 {
     REQUEST(xXF86DRIDestroyContextReq);
     REQUEST_SIZE_MATCH(xXF86DRIDestroyContextReq);
+    if (stuff->screen >= screenInfo.numScreens) {
+       client->errorValue = stuff->screen;
+       return BadValue;
+    }
 
     if (!DRIDestroyContext( screenInfo.screens[stuff->screen],
                            stuff->context)) {
@@ -348,6 +378,11 @@
 
     REQUEST(xXF86DRICreateDrawableReq);
     REQUEST_SIZE_MATCH(xXF86DRICreateDrawableReq);
+    if (stuff->screen >= screenInfo.numScreens) {
+       client->errorValue = stuff->screen;
+       return BadValue;
+    }
+
     rep.type = X_Reply;
     rep.length = 0;
     rep.sequenceNumber = client->sequence;
@@ -378,6 +413,10 @@
     REQUEST(xXF86DRIDestroyDrawableReq);
     DrawablePtr pDrawable;
     REQUEST_SIZE_MATCH(xXF86DRIDestroyDrawableReq);
+    if (stuff->screen >= screenInfo.numScreens) {
+       client->errorValue = stuff->screen;
+       return BadValue;
+    }
 
     if (!(pDrawable = (DrawablePtr)SecurityLookupDrawable(
                                                (Drawable)stuff->drawable,
@@ -409,6 +448,11 @@
 
     REQUEST(xXF86DRIGetDrawableInfoReq);
     REQUEST_SIZE_MATCH(xXF86DRIGetDrawableInfoReq);
+    if (stuff->screen >= screenInfo.numScreens) {
+       client->errorValue = stuff->screen;
+       return BadValue;
+    }
+
     rep.type = X_Reply;
     rep.length = 0;
     rep.sequenceNumber = client->sequence;
@@ -483,6 +527,11 @@
 
     REQUEST(xXF86DRIGetDeviceInfoReq);
     REQUEST_SIZE_MATCH(xXF86DRIGetDeviceInfoReq);
+    if (stuff->screen >= screenInfo.numScreens) {
+       client->errorValue = stuff->screen;
+       return BadValue;
+    }
+
     rep.type = X_Reply;
     rep.length = 0;
     rep.sequenceNumber = client->sequence;
@@ -528,6 +577,11 @@
     DrawablePtr                 pDrawable;
 
     REQUEST_SIZE_MATCH(xXF86DRIOpenFullScreenReq);
+    if (stuff->screen >= screenInfo.numScreens) {
+       client->errorValue = stuff->screen;
+       return BadValue;
+    }
+
     rep.type           = X_Reply;
     rep.length         = 0;
     rep.sequenceNumber = client->sequence;
@@ -554,6 +608,11 @@
     DrawablePtr                  pDrawable;
 
     REQUEST_SIZE_MATCH(xXF86DRICloseFullScreenReq);
+    if (stuff->screen >= screenInfo.numScreens) {
+       client->errorValue = stuff->screen;
+       return BadValue;
+    }
+
     rep.type           = X_Reply;
     rep.length         = 0;
     rep.sequenceNumber = client->sequence;
Index: glx/glxcmds.c
===================================================================
RCS file: /cvsroot/dri/xc/xc/programs/Xserver/GL/glx/glxcmds.c,v
retrieving revision 1.8
diff -u -r1.8 glxcmds.c
--- glx/glxcmds.c       25 Nov 2002 19:58:38 -0000      1.8
+++ glx/glxcmds.c       13 Dec 2002 15:52:01 -0000
@@ -761,7 +761,7 @@
     int i, p;
 
     screen = req->screen;
-    if (screen > screenInfo.numScreens) {
+    if (screen >= screenInfo.numScreens) {
        /* The client library must send a valid screen number. */
        client->errorValue = screen;
        return BadValue;
@@ -1466,7 +1466,7 @@
     ClientPtr client = cl->client;
     xGLXQueryExtensionsStringReq *req = (xGLXQueryExtensionsStringReq *) pc;
     xGLXQueryExtensionsStringReply reply;
-    GLint screen;
+    GLuint screen;
     size_t n, length;
     const char *ptr;
     char *buf;
@@ -1475,7 +1475,7 @@
     /*
     ** Check if screen exists.
     */
-    if ((screen < 0) || (screen >= screenInfo.numScreens)) {
+    if (screen >= screenInfo.numScreens) {
        client->errorValue = screen;
        return BadValue;
     }
@@ -1511,7 +1511,7 @@
     xGLXQueryServerStringReq *req = (xGLXQueryServerStringReq *) pc;
     xGLXQueryServerStringReply reply;
     int name;
-    GLint screen;
+    GLuint screen;
     size_t n, length;
     const char *ptr;
     char *buf;
@@ -1521,7 +1521,7 @@
     /*
     ** Check if screen exists.
     */
-    if ((screen < 0) || (screen >= screenInfo.numScreens)) {
+    if (screen >= screenInfo.numScreens) {
        client->errorValue = screen;
        return BadValue;
     }

Reply via email to