randr: add BUG_* checks for possible NULL pointer issue

The ‘RRCrtcNotify() and RRCrtcSet() functions are exported, so there's chance
that a buggy driver could call them with NULL parameter, leading to segfault.
Those are hard to trace, so it's better having a BUG_* check here.

| ../randr/rrcrtc.c: In function ‘RRCrtcNotify’:
| ../randr/rrcrtc.c:187:5: warning: use of NULL ‘outputs’ where non-null expected [CWE-476] [-Wanalyzer-null-argument]
|   187 |     memcpy(crtc->outputs, outputs, numOutputs * sizeof(RROutputPtr));
|       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

| ../randr/rrcrtc.c: In function ‘RRCrtcSet’:
| ../randr/rrcrtc.c:742:20: warning: dereference of NULL ‘outputs’ [CWE-476] [-Wanalyzer-null-dereference]
|   742 |         if (outputs[o]) {
|       |             ~~~~~~~^~~

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>

randr/rrcrtc.c

@@ -20,12 +20,16 @@
  * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
  * OF THIS SOFTWARE.
  */
+#include <dix-config.h>
+
+#include <X11/Xatom.h>
+
+#include "os/bug_priv.h"
 
 #include "randrstr_priv.h"
 #include "swaprep.h"
 #include "mipointer.h"
 
-#include <X11/Xatom.h>
 
 RESTYPE RRCrtcType = 0;
 
@@ -181,10 +185,13 @@ RRCrtcNotify(RRCrtcPtr crtc,
         crtc->outputs = newoutputs;
         crtc->numOutputs = numOutputs;
     }
+
     /*
      * Copy the new list of outputs into the crtc
      */
+    BUG_RETURN_VAL(outputs == NULL, FALSE);
     memcpy(crtc->outputs, outputs, numOutputs * sizeof(RROutputPtr));
+
     /*
      * Update remaining crtc fields
      */
@@ -735,6 +742,8 @@ RRCrtcSet(RRCrtcPtr crtc,
     Bool crtcChanged;
     int  o;
 
+    BUG_RETURN_VAL(outputs == NULL, FALSE);
+
     rrScrPriv(pScreen);
 
     crtcChanged = FALSE;

randr/rroutput.c

@@ -119,7 +119,6 @@ RROutputCreate(ScreenPtr pScreen,
 Bool
 RROutputSetClones(RROutputPtr output, RROutputPtr * clones, int numClones)
 {
-    RROutputPtr *newClones;
     int i;
 
     if (numClones == output->numClones) {
@@ -129,15 +128,16 @@ RROutputSetClones(RROutputPtr output, RROutputPtr * clones, int numClones)
         if (i == numClones)
             return TRUE;
     }
+
+    RROutputPtr *newClones = NULL;
     if (numClones) {
         newClones = xallocarray(numClones, sizeof(RROutputPtr));
         if (!newClones)
             return FALSE;
+        memcpy(newClones, clones, numClones * sizeof(RROutputPtr));
     }
-    else
-        newClones = NULL;
+
     free(output->clones);
-    memcpy(newClones, clones, numClones * sizeof(RROutputPtr));
     output->clones = newClones;
     output->numClones = numClones;
     RROutputChanged(output, TRUE);
@@ -148,7 +148,6 @@ Bool
 RROutputSetModes(RROutputPtr output,
                  RRModePtr * modes, int numModes, int numPreferred)
 {
-    RRModePtr *newModes;
     int i;
 
     if (numModes == output->numModes && numPreferred == output->numPreferred) {
@@ -162,19 +161,19 @@ RROutputSetModes(RROutputPtr output,
         }
     }
 
+    RRModePtr *newModes = NULL;
     if (numModes) {
         newModes = xallocarray(numModes, sizeof(RRModePtr));
         if (!newModes)
             return FALSE;
+        memcpy(newModes, modes, numModes * sizeof(RRModePtr));
     }
-    else
-        newModes = NULL;
+
     if (output->modes) {
         for (i = 0; i < output->numModes; i++)
             RRModeDestroy(output->modes[i]);
         free(output->modes);
     }
-    memcpy(newModes, modes, numModes * sizeof(RRModePtr));
     output->modes = newModes;
     output->numModes = numModes;
     output->numPreferred = numPreferred;
@@ -251,7 +250,6 @@ RROutputDeleteUserMode(RROutputPtr output, RRModePtr mode)
 Bool
 RROutputSetCrtcs(RROutputPtr output, RRCrtcPtr * crtcs, int numCrtcs)
 {
-    RRCrtcPtr *newCrtcs;
     int i;
 
     if (numCrtcs == output->numCrtcs) {
@@ -261,15 +259,16 @@ RROutputSetCrtcs(RROutputPtr output, RRCrtcPtr * crtcs, int numCrtcs)
         if (i == numCrtcs)
             return TRUE;
     }
+
+    RRCrtcPtr *newCrtcs = NULL;
     if (numCrtcs) {
         newCrtcs = xallocarray(numCrtcs, sizeof(RRCrtcPtr));
         if (!newCrtcs)
             return FALSE;
+        memcpy(newCrtcs, crtcs, numCrtcs * sizeof(RRCrtcPtr));
     }
-    else
-        newCrtcs = NULL;
+
     free(output->crtcs);
-    memcpy(newCrtcs, crtcs, numCrtcs * sizeof(RRCrtcPtr));
     output->crtcs = newCrtcs;
     output->numCrtcs = numCrtcs;
     RROutputChanged(output, TRUE);
@@ -439,14 +438,12 @@ ProcRRGetOutputInfo(ClientPtr client)
     REQUEST(xRRGetOutputInfoReq);
     xRRGetOutputInfoReply rep;
     RROutputPtr output;
-    CARD8 *extra;
     unsigned long extraLen;
     ScreenPtr pScreen;
     rrScrPrivPtr pScrPriv;
     RRCrtc *crtcs;
     RRMode *modes;
     RROutput *clones;
-    char *name;
     int i;
     Bool leased;
 
@@ -458,6 +455,8 @@ ProcRRGetOutputInfo(ClientPtr client)
     pScreen = output->pScreen;
     pScrPriv = rrGetScrPriv(pScreen);
 
+    CARD8 *extra = NULL;
+
     if (leased) {
         rep = (xRRGetOutputInfoReply) {
             .type = X_Reply,
@@ -470,16 +469,14 @@ ProcRRGetOutputInfo(ClientPtr client)
             .nameLength = output->nameLength
         };
         extraLen = bytes_to_int32(rep.nameLength) << 2;
-        if (extraLen) {
-            rep.length += bytes_to_int32(extraLen);
-            extra = calloc(1, extraLen);
-            if (!extra)
-                return BadAlloc;
-        }
-        else
-            extra = NULL;
+        if (!extraLen)
+            goto sendout;
 
-        name = (char *) extra;
+        rep.length += bytes_to_int32(extraLen);
+        extra = calloc(1, extraLen);
+        if (!extra)
+            return BadAlloc;
+        memcpy(extra, output->name, output->nameLength);
     } else {
         rep = (xRRGetOutputInfoReply) {
             .type = X_Reply,
@@ -502,25 +499,26 @@ ProcRRGetOutputInfo(ClientPtr client)
                      output->numModes + output->numUserModes +
                      output->numClones + bytes_to_int32(rep.nameLength)) << 2);
 
-        if (extraLen) {
-            rep.length += bytes_to_int32(extraLen);
-            extra = calloc(1, extraLen);
-            if (!extra)
-                return BadAlloc;
-        }
-        else
-            extra = NULL;
+        if (!extraLen)
+            goto sendout;
+
+        rep.length += bytes_to_int32(extraLen);
+        extra = calloc(1, extraLen);
+        if (!extra)
+            return BadAlloc;
 
         crtcs = (RRCrtc *) extra;
         modes = (RRMode *) (crtcs + output->numCrtcs);
         clones = (RROutput *) (modes + output->numModes + output->numUserModes);
-        name = (char *) (clones + output->numClones);
+
+        memcpy((clones + output->numClones), output->name, output->nameLength);
 
         for (i = 0; i < output->numCrtcs; i++) {
             crtcs[i] = output->crtcs[i]->id;
             if (client->swapped)
                 swapl(&crtcs[i]);
         }
+
         for (i = 0; i < output->numModes + output->numUserModes; i++) {
             if (i < output->numModes)
                 modes[i] = output->modes[i]->mode.id;
@@ -535,7 +533,8 @@ ProcRRGetOutputInfo(ClientPtr client)
                 swapl(&clones[i]);
         }
     }
-    memcpy(name, output->name, output->nameLength);
+
+sendout:
     if (client->swapped) {
         swaps(&rep.sequenceNumber);
         swapl(&rep.length);
@@ -549,11 +548,10 @@ ProcRRGetOutputInfo(ClientPtr client)
         swaps(&rep.nClones);
         swaps(&rep.nameLength);
     }
+
     WriteToClient(client, sizeof(xRRGetOutputInfoReply), &rep);
-    if (extraLen) {
-        WriteToClient(client, extraLen, extra);
-        free(extra);
-    }
+    WriteToClient(client, extraLen, extra);
+    free(extra);
 
     return Success;
 }

randr/rrproperty.c

@@ -359,7 +359,6 @@ RRConfigureOutputProperty(RROutputPtr output, Atom property,
 {
     RRPropertyPtr prop = RRQueryOutputProperty(output, property);
     Bool add = FALSE;
-    INT32 *new_values;
 
     if (!prop) {
         prop = RRCreateOutputProperty(property);
@@ -379,14 +378,17 @@ RRConfigureOutputProperty(RROutputPtr output, Atom property,
         return BadMatch;
     }
 
-    new_values = xallocarray(num_values, sizeof(INT32));
-    if (!new_values && num_values) {
-        if (add)
-            RRDestroyOutputProperty(prop);
-        return BadAlloc;
-    }
-    if (num_values)
+    INT32 *new_values = NULL;
+
+    if (num_values) {
+        new_values = calloc(num_values, sizeof(INT32));
+        if (!new_values) {
+            if (add)
+                RRDestroyOutputProperty(prop);
+            return BadAlloc;
+        }
         memcpy(new_values, values, num_values * sizeof(INT32));
+    }
 
     /*
      * Property moving from pending to non-pending

randr/rrproviderproperty.c

@@ -327,7 +327,6 @@ RRConfigureProviderProperty(RRProviderPtr provider, Atom property,
 {
     RRPropertyPtr prop = RRQueryProviderProperty(provider, property);
     Bool add = FALSE;
-    INT32 *new_values;
 
     if (!prop) {
         prop = RRCreateProviderProperty(property);
@@ -347,14 +346,16 @@ RRConfigureProviderProperty(RRProviderPtr provider, Atom property,
         return BadMatch;
     }
 
-    new_values = xallocarray(num_values, sizeof(INT32));
-    if (!new_values && num_values) {
-        if (add)
-            RRDestroyProviderProperty(prop);
-        return BadAlloc;
-    }
-    if (num_values)
+    INT32 *new_values = NULL;
+    if (num_values) {
+        new_values = calloc(num_values, sizeof(INT32));
+        if (!new_values) {
+            if (add)
+                RRDestroyProviderProperty(prop);
+            return BadAlloc;
+        }
         memcpy(new_values, values, num_values * sizeof(INT32));
+    }
 
     /*
      * Property moving from pending to non-pending
@@ -384,7 +385,7 @@ int
 ProcRRListProviderProperties(ClientPtr client)
 {
     REQUEST(xRRListProviderPropertiesReq);
-    Atom *pAtoms = NULL, *temppAtoms;
+    Atom *pAtoms = NULL;
     int numProps = 0;
     RRProviderPtr provider;
     RRPropertyPtr prop;
@@ -395,9 +396,20 @@ ProcRRListProviderProperties(ClientPtr client)
 
     for (prop = provider->properties; prop; prop = prop->next)
         numProps++;
-    if (numProps)
-        if (!(pAtoms = xallocarray(numProps, sizeof(Atom))))
+
+    const Bool swapped = client->swapped;
+
+    if (numProps) {
+        if (!(pAtoms = calloc(numProps, sizeof(Atom))))
             return BadAlloc;
+        Atom *temppAtoms = pAtoms;
+        for (prop = provider->properties; prop; prop = prop->next) {
+            *temppAtoms = prop->propertyName;
+            if (swapped)
+                swapl(temppAtoms);
+            temppAtoms++;
+        }
+    }
 
     xRRListProviderPropertiesReply rep = {
         .type = X_Reply,
@@ -405,21 +417,16 @@ ProcRRListProviderProperties(ClientPtr client)
         .length = bytes_to_int32(numProps * sizeof(Atom)),
         .nAtoms = numProps
     };
-    if (client->swapped) {
+    if (swapped) {
         swaps(&rep.sequenceNumber);
         swapl(&rep.length);
         swaps(&rep.nAtoms);
     }
-    temppAtoms = pAtoms;
-    for (prop = provider->properties; prop; prop = prop->next)
-        *temppAtoms++ = prop->propertyName;
 
     WriteToClient(client, sizeof(xRRListProviderPropertiesReply), (char *) &rep);
-    if (numProps) {
-        client->pSwapReplyFunc = (ReplySwapPtr) Swap32Write;
-        WriteSwappedDataToClient(client, numProps * sizeof(Atom), pAtoms);
-        free(pAtoms);
-    }
+    WriteToClient(client, numProps * sizeof(Atom), pAtoms);
+
+    free(pAtoms);
     return Success;
 }
 
@@ -429,7 +436,6 @@ ProcRRQueryProviderProperty(ClientPtr client)
     REQUEST(xRRQueryProviderPropertyReq);
     RRProviderPtr provider;
     RRPropertyPtr prop;
-    char *extra = NULL;
 
     REQUEST_SIZE_MATCH(xRRQueryProviderPropertyReq);
 
@@ -439,12 +445,6 @@ ProcRRQueryProviderProperty(ClientPtr client)
     if (!prop)
         return BadName;
 
-    if (prop->num_valid) {
-        extra = xallocarray(prop->num_valid, sizeof(INT32));
-        if (!extra)
-            return BadAlloc;
-    }
-
     xRRQueryProviderPropertyReply rep = {
         .type = X_Reply,
         .sequenceNumber = client->sequence,
@@ -459,11 +459,10 @@ ProcRRQueryProviderProperty(ClientPtr client)
     }
     WriteToClient(client, sizeof(xRRQueryProviderPropertyReply), (char *) &rep);
     if (prop->num_valid) {
-        memcpy(extra, prop->valid_values, prop->num_valid * sizeof(INT32));
-        client->pSwapReplyFunc = (ReplySwapPtr) Swap32Write;
-        WriteSwappedDataToClient(client, prop->num_valid * sizeof(INT32),
-                                 extra);
-        free(extra);
+        if (client->swapped)
+            CopySwap32Write(client, prop->num_valid * sizeof(INT32), (CARD32*)prop->valid_values);
+        else
+            WriteToClient(client, prop->num_valid * sizeof(INT32), prop->valid_values);
     }
     return Success;
 }

randr/rrscreen.c

@@ -488,10 +488,6 @@ rrGetScreenResources(ClientPtr client, Bool query)
     CARD8 *extra = NULL;
     unsigned long extraLen = 0;
     int i, rc, has_primary = 0;
-    RRCrtc *crtcs;
-    RROutput *outputs;
-    xRRModeInfo *modeinfos;
-    CARD8 *names;
 
     REQUEST_SIZE_MATCH(xRRGetScreenResourcesReq);
     rc = dixLookupWindow(&pWin, stuff->window, client, DixGetAttrAccess);
@@ -543,20 +539,19 @@ rrGetScreenResources(ClientPtr client, Bool query)
                       bytes_to_int32(rep.nbytesNames));
 
         extraLen = rep.length << 2;
-        if (extraLen) {
-            extra = calloc(1, extraLen);
-            if (!extra) {
-                free(modes);
-                return BadAlloc;
-            }
-        }
-        else
-            extra = NULL;
+        if (!extraLen)
+            goto finish;
 
-        crtcs = (RRCrtc *) extra;
-        outputs = (RROutput *) (crtcs + pScrPriv->numCrtcs);
-        modeinfos = (xRRModeInfo *) (outputs + pScrPriv->numOutputs);
-        names = (CARD8 *) (modeinfos + num_modes);
+        extra = calloc(1, extraLen);
+        if (!extra) {
+            free(modes);
+            return BadAlloc;
+        }
+
+        RRCrtc *crtcs = (RRCrtc *) extra;
+        RROutput *outputs = (RROutput *) (crtcs + pScrPriv->numCrtcs);
+        xRRModeInfo *modeinfos = (xRRModeInfo *) (outputs + pScrPriv->numOutputs);
+        CARD8* names = (CARD8 *) (modeinfos + num_modes);
 
         if (pScrPriv->primaryOutput && pScrPriv->primaryOutput->crtc) {
             has_primary = 1;
@@ -604,8 +599,9 @@ rrGetScreenResources(ClientPtr client, Bool query)
             memcpy(names, mode->name, mode->mode.nameLength);
             names += mode->mode.nameLength;
         }
-        free(modes);
         assert(bytes_to_int32((char *) names - (char *) extra) == rep.length);
+finish:
+        free(modes);
     }
 
     if (client->swapped) {
@@ -777,8 +773,6 @@ ProcRRGetScreenInfo(ClientPtr client)
     }
     else {
         int i, j;
-        xScreenSizes *size;
-        CARD16 *rates;
         CARD8 *data8;
         Bool has_rate = RRClientKnowsRates(client);
         RR10DataPtr pData;
@@ -806,21 +800,20 @@ ProcRRGetScreenInfo(ClientPtr client)
         if (has_rate)
             extraLen += rep.nrateEnts * sizeof(CARD16);
 
-        if (extraLen) {
-            extra = (CARD8 *) malloc(extraLen);
-            if (!extra) {
-                free(pData);
-                return BadAlloc;
-            }
+        if (!extraLen)
+            goto finish; // no extra payload
+
+        extra = calloc(1, extraLen);
+        if (!extra) {
+            free(pData);
+            return BadAlloc;
         }
-        else
-            extra = NULL;
 
         /*
          * First comes the size information
          */
-        size = (xScreenSizes *) extra;
-        rates = (CARD16 *) (size + rep.nSizes);
+        xScreenSizes *size = (xScreenSizes *) extra;
+        CARD16 *rates = (CARD16 *) (size + rep.nSizes);
         for (i = 0; i < pData->nsize; i++) {
             pSize = &pData->sizes[i];
             size->widthInPixels = pSize->width;
@@ -849,7 +842,6 @@ ProcRRGetScreenInfo(ClientPtr client)
                 }
             }
         }
-        free(pData);
 
         data8 = (CARD8 *) rates;
 
@@ -857,6 +849,9 @@ ProcRRGetScreenInfo(ClientPtr client)
             FatalError("RRGetScreenInfo bad extra len %ld != %ld\n",
                        (unsigned long) (data8 - (CARD8 *) extra), extraLen);
         rep.length = bytes_to_int32(extraLen);
+
+finish:
+        free(pData);
     }
     if (client->swapped) {
         swaps(&rep.sequenceNumber);