[PATCH] xfree86: Copy mode->name for desired mode and current mode

Maarten Lankhorst maarten.lankhorst at canonical.com
Thu Jun 21 06:49:30 PDT 2012


I noticed that on EnterVT after a suspend valgrind would complain
due to ->name being freed earlier, after X sometimes crashed on
returning from suspend. Attached patch fixes this for me on x1.11
which seems to be identical to x-server.git in almost every way.
It has been reapplied to X server

Alternatively this member could be made a static array instead
of a pointer, but this would require a video abi bump.

I'm still a bit unfamiliar to the core x server so any feedback is welcome.

The specific valgrind error happens a lot later, long after the fact:

==30042== Invalid read of size 1
==30042==    at 0x4C2BFD4: __GI_strlen (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==30042==    by 0x673E7F5: strdup (strdup.c:42)
==30042==    by 0x29673D: XNFstrdup (utils.c:1119)
==30042==    by 0x1CEC08: xf86DuplicateMode (xf86Modes.c:209)
==30042==    by 0x1C865E: xf86CrtcSetModeTransform (xf86Crtc.c:276)
==30042==    by 0x1C8F30: xf86SetDesiredModes (xf86Crtc.c:2674)
==30042==    by 0x8C5710E: RADEONEnterVT (radeon_driver.c:6269)
==30042==    by 0x1D115F: xf86RandR12EnterVT (xf86RandR12.c:1764)
==30042==    by 0x193967: xf86Wakeup (xf86Events.c:527)
==30042==    by 0x15A7FA: WakeupHandler (dixutils.c:428)
==30042==    by 0x28DDA5: WaitForSomething (WaitFor.c:235)
==30042==    by 0x156601: Dispatch (dispatch.c:366)
==30042==  Address 0x98bebf1 is 1 bytes inside a block of size 9 free'd
==30042==    at 0x4C2A82E: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==30042==    by 0x1AAA51: xf86DeleteMode (xf86Mode.c:1985)
==30042==    by 0x1C7617: xf86ProbeOutputModes (xf86Crtc.c:1575)
==30042==    by 0x1D06B3: xf86RandR12GetInfo12 (xf86RandR12.c:1538)
==30042==    by 0x20285C: RRGetInfo (rrinfo.c:202)
==30042==    by 0x20743D: rrGetScreenResources (rrscreen.c:337)
==30042==    by 0x1568B0: Dispatch (dispatch.c:442)

However, if you look you can find the problem happen a lot earlier,
if you add free(mode->name[1]); to get where the allocation occured:

==30042== Invalid free() / delete / delete[] / realloc()
==30042==    at 0x4C2A82E: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==30042==    by 0x1AAA48: xf86DeleteMode (xf86Mode.c:1984)
==30042==    by 0x1C7617: xf86ProbeOutputModes (xf86Crtc.c:1575)
==30042==    by 0x1D06B3: xf86RandR12GetInfo12 (xf86RandR12.c:1538)
==30042==    by 0x20285C: RRGetInfo (rrinfo.c:202)
==30042==    by 0x20743D: rrGetScreenResources (rrscreen.c:337)
==30042==    by 0x1568B0: Dispatch (dispatch.c:442)
==30042==    by 0x1456D9: main (main.c:288)
==30042==  Address 0x98bebf1 is 1 bytes inside a block of size 9 alloc'd
==30042==    at 0x4C2B6CD: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==30042==    by 0x296638: XNFalloc (utils.c:1048)
==30042==    by 0x8CBA432: RADEONFPNativeMode (radeon_modes.c:148)
==30042==    by 0x8CBB333: RADEONProbeOutputModes (radeon_modes.c:528)
==30042==    by 0x8CB6336: radeon_get_modes (radeon_output.c:1297)
==30042==    by 0x1C7706: xf86ProbeOutputModes (xf86Crtc.c:1614)
==30042==    by 0x1C9334: xf86InitialConfiguration (xf86Crtc.c:2399)
==30042==    by 0x8C4DFE3: RADEONPreInit (radeon_driver.c:3203)
==30042==    by 0x197137: InitOutput (xf86Init.c:599)
==30042==    by 0x14550C: main (main.c:205)

Signed-off-by: Maarten Lankhorst <maarten.lankhorst at canonical.com>

---
diff --git a/hw/xfree86/modes/xf86Crtc.c b/hw/xfree86/modes/xf86Crtc.c
index 2c8878f..8531b7b 100644
--- a/hw/xfree86/modes/xf86Crtc.c
+++ b/hw/xfree86/modes/xf86Crtc.c
@@ -362,6 +362,8 @@ xf86CrtcSetModeTransform(xf86CrtcPtr crtc, DisplayModePtr mode,
 
  done:
     if (ret) {
+        crtc->mode.name = strdup(mode->name);
+        free(saved_mode.name);
         crtc->active = TRUE;
         if (scrn->pScreen)
             xf86CrtcSetScreenSubpixelOrder(scrn->pScreen);
@@ -2417,6 +2419,7 @@ xf86InitialConfiguration(ScrnInfoPtr scrn, Bool canGrow)
         xf86CrtcPtr crtc = config->crtc[c];
 
         crtc->enabled = FALSE;
+        free(&crtc->desiredMode.name);
         memset(&crtc->desiredMode, '\0', sizeof(crtc->desiredMode));
         /* Set default gamma for all crtc's. */
         /* This is done to avoid problems later on with cloned outputs. */
@@ -2437,6 +2440,7 @@ xf86InitialConfiguration(ScrnInfoPtr scrn, Bool canGrow)
 
         if (mode && crtc) {
             crtc->desiredMode = *mode;
+            crtc->desiredMode.name = strdup(mode->name);
             crtc->desiredRotation = output->initial_rotation;
             crtc->desiredX = output->initial_x;
             crtc->desiredY = output->initial_y;
@@ -2613,6 +2617,7 @@ xf86SetDesiredModes(ScrnInfoPtr scrn)
             continue;
 
         /* Mark that we'll need to re-set the mode for sure */
+        free(crtc->mode.name);
         memset(&crtc->mode, 0, sizeof(crtc->mode));
         if (!crtc->desiredMode.CrtcHDisplay) {
             DisplayModePtr mode =
@@ -2620,7 +2625,9 @@ xf86SetDesiredModes(ScrnInfoPtr scrn)
 
             if (!mode)
                 return FALSE;
+            free(crtc->desiredMode.name);
             crtc->desiredMode = *mode;
+            crtc->desiredMode.name = strdup(mode->name);
             crtc->desiredRotation = RR_Rotate_0;
             crtc->desiredTransformPresent = FALSE;
             crtc->desiredX = 0;
@@ -2759,7 +2766,9 @@ xf86SetSingleMode(ScrnInfoPtr pScrn, DisplayModePtr desired, Rotation rotation)
         if (!xf86CrtcSetModeTransform(crtc, crtc_mode, rotation, NULL, 0, 0))
             ok = FALSE;
         else {
+            free(crtc->desiredMode.name);
             crtc->desiredMode = *crtc_mode;
+            crtc->desiredMode.name = strdup(crtc_mode->name);
             crtc->desiredRotation = rotation;
             crtc->desiredTransformPresent = FALSE;
             crtc->desiredX = 0;
@@ -2854,6 +2863,7 @@ xf86DisableUnusedFunctions(ScrnInfoPtr pScrn)
 
         if (!crtc->enabled) {
             crtc->funcs->dpms(crtc, DPMSModeOff);
+            free(crtc->mode.name);
             memset(&crtc->mode, 0, sizeof(crtc->mode));
             xf86RotateDestroy(crtc);
             crtc->active = FALSE;
diff --git a/hw/xfree86/modes/xf86RandR12.c b/hw/xfree86/modes/xf86RandR12.c
index 59b6f82..48cef30 100644
--- a/hw/xfree86/modes/xf86RandR12.c
+++ b/hw/xfree86/modes/xf86RandR12.c
@@ -1216,7 +1216,9 @@ xf86RandR12CrtcSet(ScreenPtr pScreen,
             /*
              * Save the last successful setting for EnterVT
              */
+            free(crtc->desiredMode.name);
             crtc->desiredMode = mode;
+            crtc->desiredMode.name = strdup(mode.name);
             crtc->desiredRotation = rotation;
             if (transform) {
                 crtc->desiredTransform = *transform;




More information about the xorg-devel mailing list