subject:"\[PATCH\] xf86\/modes\: Defang stale pointers when copying modes"

Re: [PATCH] xf86/modes: Defang stale pointers when copying modes

2012-05-31 Thread Chris Wilson

On Wed, 30 May 2012 14:25:40 -0700, Keith Packard kei...@keithp.com wrote:
 Chris Wilson ch...@chris-wilson.co.uk writes:
 
  +extern _X_EXPORT void
  +xf86InternMode(DisplayModePtr intern, const DisplayModeRec * pMode);
 
 The name doesn't seem very descriptive to me -- what is 'Intern'
 supposed to mean?

xf86InternalizeMode? xf86InitModeFromCopy? xf86CopyModeToStatic?

   /**
  + * Fills in a copy of mode, removing all stale pointer references.

Fills in an embedded copy of mode, removing all stale pointer
references and copying only the modeline configuration. Subsequently
comparing the copy with the original with xf86ModesEqual() will
return TRUE.

  + */
  +void
  +xf86InternMode(DisplayModePtr intern, const DisplayModeRec *mode)
  +{
  +*intern = *mode;
  +intern-prev = intern-next = NULL;
  +intern-name = NULL;
  +}

 What should be done with -Private? If this mode was ever actually handed
 to a driver, it might not be NULL...

/* And drop the backend's privates. */
intern-PrivSize = 0;
intern-PrivFlags = 0;
intern-Private = 0;

-- 
Chris Wilson, Intel Open Source Technology Centre
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] xf86/modes: Defang stale pointers when copying modes

2012-05-31 Thread Keith Packard

Chris Wilson ch...@chris-wilson.co.uk writes:

 On Wed, 30 May 2012 14:25:40 -0700, Keith Packard kei...@keithp.com wrote:
 Chris Wilson ch...@chris-wilson.co.uk writes:
 
  +extern _X_EXPORT void
  +xf86InternMode(DisplayModePtr intern, const DisplayModeRec * pMode);
 
 The name doesn't seem very descriptive to me -- what is 'Intern'
 supposed to mean?

 xf86InternalizeMode? xf86InitModeFromCopy? xf86CopyModeToStatic?

xf86SaveModeContents? (s/Contents/something-more-descriptive/?)

 What should be done with -Private? If this mode was ever actually handed
 to a driver, it might not be NULL...

 /* And drop the backend's privates. */
 intern-PrivSize = 0;
 intern-PrivFlags = 0;
 intern-Private = 0;

Yeah, I don't think there's anything more useful to be done here.

-- 
keith.pack...@intel.com


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

Re: [PATCH] xf86/modes: Defang stale pointers when copying modes

2012-05-30 Thread Keith Packard

Chris Wilson ch...@chris-wilson.co.uk writes:

 +extern _X_EXPORT void
 +xf86InternMode(DisplayModePtr intern, const DisplayModeRec * pMode);

The name doesn't seem very descriptive to me -- what is 'Intern'
supposed to mean?


  /**
 + * Fills in a copy of mode, removing all stale pointer references.
 + */
 +void
 +xf86InternMode(DisplayModePtr intern, const DisplayModeRec *mode)
 +{
 +*intern = *mode;
 +intern-prev = intern-next = NULL;
 +intern-name = NULL;
 +}

What should be done with -Private? If this mode was ever actually handed
to a driver, it might not be NULL...

-- 
keith.pack...@intel.com


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

[PATCH] xf86/modes: Defang stale pointers when copying modes

2012-05-29 Thread Chris Wilson

We stash a copy of the desiredMode on the crtc so that we can restore it
after a vt switch. This copy is a simple memcpy and so also stashes a
references to the pointers contained within the desiredMode. Those
pointers are freed the next time the outputs are probed and mode list
rebuilt, resulting in us chasing those dangling pointers on the next
mode switch.

==22787== Invalid read of size 1
==22787==at 0x40293C2: __GI_strlen (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==22787==by 0x668F875: strdup (strdup.c:42)
==22787==by 0x5DBA00: XNFstrdup (utils.c:1124)
==22787==by 0x4D72ED: xf86DuplicateMode (xf86Modes.c:209)
==22787==by 0x4CA848: xf86CrtcSetModeTransform (xf86Crtc.c:276)
==22787==by 0x4D05B4: xf86SetDesiredModes (xf86Crtc.c:2677)
==22787==by 0xA7479D0: sna_create_screen_resources
(sna_driver.c:220)
==22787==by 0x4CB914: xf86CrtcCreateScreenResources (xf86Crtc.c:725)
==22787==by 0x425498: main (main.c:216)
==22787==  Address 0x72c60e0 is 0 bytes inside a block of size 9 free'd
==22787==at 0x4027AAE: free (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==22787==by 0x4A547E: xf86DeleteMode (xf86Mode.c:1984)
==22787==by 0x4CD84F: xf86ProbeOutputModes (xf86Crtc.c:1578)
==22787==by 0x4DC405: xf86RandR12GetInfo12 (xf86RandR12.c:1537)
==22787==by 0x518119: RRGetInfo (rrinfo.c:202)
==22787==by 0x51D997: rrGetScreenResources (rrscreen.c:335)
==22787==by 0x51E0D0: ProcRRGetScreenResources (rrscreen.c:475)
==22787==by 0x513852: ProcRRDispatch (randr.c:493)
==22787==by 0x4346DB: Dispatch (dispatch.c:439)
==22787==by 0x4256E4: main (main.c:287)

v2: Rebase upon whitespaces changes.

Reported-by: Zdenek Kabelac zdenek.kabe...@gmail.com
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=36108
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
---
 hw/xfree86/common/xf86.h   |2 ++
 hw/xfree86/modes/xf86Crtc.c|6 +++---
 hw/xfree86/modes/xf86Modes.c   |   11 +++
 hw/xfree86/modes/xf86RandR12.c |2 +-
 4 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/hw/xfree86/common/xf86.h b/hw/xfree86/common/xf86.h
index 0eb000d..6e5e768 100644
--- a/hw/xfree86/common/xf86.h
+++ b/hw/xfree86/common/xf86.h
@@ -417,6 +417,8 @@ extern _X_EXPORT void
 xf86SetModeCrtc(DisplayModePtr p, int adjustFlags);
 extern _X_EXPORT DisplayModePtr
 xf86DuplicateMode(const DisplayModeRec * pMode);
+extern _X_EXPORT void
+xf86InternMode(DisplayModePtr intern, const DisplayModeRec * pMode);
 extern _X_EXPORT DisplayModePtr
 xf86DuplicateModes(ScrnInfoPtr pScrn, DisplayModePtr modeList);
 extern _X_EXPORT Bool
diff --git a/hw/xfree86/modes/xf86Crtc.c b/hw/xfree86/modes/xf86Crtc.c
index ab6ed5e..c8c2d72 100644
--- a/hw/xfree86/modes/xf86Crtc.c
+++ b/hw/xfree86/modes/xf86Crtc.c
@@ -2436,7 +2436,7 @@ xf86InitialConfiguration(ScrnInfoPtr scrn, Bool canGrow)
 xf86CrtcPtr crtc = crtcs[o];
 
 if (mode  crtc) {
-crtc-desiredMode = *mode;
+xf86InternMode(crtc-desiredMode, mode);
 crtc-desiredRotation = output-initial_rotation;
 crtc-desiredX = output-initial_x;
 crtc-desiredY = output-initial_y;
@@ -2620,7 +2620,7 @@ xf86SetDesiredModes(ScrnInfoPtr scrn)
 
 if (!mode)
 return FALSE;
-crtc-desiredMode = *mode;
+xf86InternMode(crtc-desiredMode, mode);
 crtc-desiredRotation = RR_Rotate_0;
 crtc-desiredTransformPresent = FALSE;
 crtc-desiredX = 0;
@@ -2759,7 +2759,7 @@ xf86SetSingleMode(ScrnInfoPtr pScrn, DisplayModePtr 
desired, Rotation rotation)
 if (!xf86CrtcSetModeTransform(crtc, crtc_mode, rotation, NULL, 0, 0))
 ok = FALSE;
 else {
-crtc-desiredMode = *crtc_mode;
+xf86InternMode(crtc-desiredMode, crtc_mode);
 crtc-desiredRotation = rotation;
 crtc-desiredTransformPresent = FALSE;
 crtc-desiredX = 0;
diff --git a/hw/xfree86/modes/xf86Modes.c b/hw/xfree86/modes/xf86Modes.c
index 2a6d267..bfdc311 100644
--- a/hw/xfree86/modes/xf86Modes.c
+++ b/hw/xfree86/modes/xf86Modes.c
@@ -191,6 +191,17 @@ xf86SetModeCrtc(DisplayModePtr p, int adjustFlags)
 }
 
 /**
+ * Fills in a copy of mode, removing all stale pointer references.
+ */
+void
+xf86InternMode(DisplayModePtr intern, const DisplayModeRec *mode)
+{
+*intern = *mode;
+intern-prev = intern-next = NULL;
+intern-name = NULL;
+}
+
+/**
  * Allocates and returns a copy of pMode, including pointers within pMode.
  */
 DisplayModePtr
diff --git a/hw/xfree86/modes/xf86RandR12.c b/hw/xfree86/modes/xf86RandR12.c
index e6b2052..0dfca5d 100644
--- a/hw/xfree86/modes/xf86RandR12.c
+++ b/hw/xfree86/modes/xf86RandR12.c
@@ -1216,7 +1216,7 @@ xf86RandR12CrtcSet(ScreenPtr pScreen,
 /*
  * Save the last successful setting for EnterVT
  */
-crtc-desiredMode = mode;
+

[PATCH] xf86/modes: Defang stale pointers when copying modes

2012-03-16 Thread Chris Wilson

We stash a copy of the desiredMode on the crtc so that we can restore it
after a vt switch. This copy is a simple memcpy and so also stashes a
references to the pointers contained within the desiredMode. Those
pointers are freed the next time the outputs are probed and mode list
rebuilt, resulting in us chasing those dangling pointers on the next
mode switch.

==22787== Invalid read of size 1
==22787==at 0x40293C2: __GI_strlen (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==22787==by 0x668F875: strdup (strdup.c:42)
==22787==by 0x5DBA00: XNFstrdup (utils.c:1124)
==22787==by 0x4D72ED: xf86DuplicateMode (xf86Modes.c:209)
==22787==by 0x4CA848: xf86CrtcSetModeTransform (xf86Crtc.c:276)
==22787==by 0x4D05B4: xf86SetDesiredModes (xf86Crtc.c:2677)
==22787==by 0xA7479D0: sna_create_screen_resources
(sna_driver.c:220)
==22787==by 0x4CB914: xf86CrtcCreateScreenResources (xf86Crtc.c:725)
==22787==by 0x425498: main (main.c:216)
==22787==  Address 0x72c60e0 is 0 bytes inside a block of size 9 free'd
==22787==at 0x4027AAE: free (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==22787==by 0x4A547E: xf86DeleteMode (xf86Mode.c:1984)
==22787==by 0x4CD84F: xf86ProbeOutputModes (xf86Crtc.c:1578)
==22787==by 0x4DC405: xf86RandR12GetInfo12 (xf86RandR12.c:1537)
==22787==by 0x518119: RRGetInfo (rrinfo.c:202)
==22787==by 0x51D997: rrGetScreenResources (rrscreen.c:335)
==22787==by 0x51E0D0: ProcRRGetScreenResources (rrscreen.c:475)
==22787==by 0x513852: ProcRRDispatch (randr.c:493)
==22787==by 0x4346DB: Dispatch (dispatch.c:439)
==22787==by 0x4256E4: main (main.c:287)

Reported-by: Zdenek Kabelac zdenek.kabe...@gmail.com
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=36108
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
---
 hw/xfree86/common/xf86.h   |1 +
 hw/xfree86/modes/xf86Crtc.c|6 +++---
 hw/xfree86/modes/xf86Modes.c   |   11 +++
 hw/xfree86/modes/xf86RandR12.c |2 +-
 4 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/hw/xfree86/common/xf86.h b/hw/xfree86/common/xf86.h
index b711f05..f83d66f 100644
--- a/hw/xfree86/common/xf86.h
+++ b/hw/xfree86/common/xf86.h
@@ -326,6 +326,7 @@ extern _X_EXPORT double xf86ModeVRefresh(const 
DisplayModeRec *mode);
 extern _X_EXPORT void xf86SetModeDefaultName(DisplayModePtr mode);
 extern _X_EXPORT void xf86SetModeCrtc(DisplayModePtr p, int adjustFlags);
 extern _X_EXPORT DisplayModePtr xf86DuplicateMode(const DisplayModeRec *pMode);
+extern _X_EXPORT void xf86InternMode(DisplayModeRec *intern, const 
DisplayModeRec *mode);
 extern _X_EXPORT DisplayModePtr xf86DuplicateModes(ScrnInfoPtr pScrn, 
DisplayModePtr modeList);
 extern _X_EXPORT Bool xf86ModesEqual(const DisplayModeRec *pMode1,
const DisplayModeRec *pMode2);
diff --git a/hw/xfree86/modes/xf86Crtc.c b/hw/xfree86/modes/xf86Crtc.c
index da9db34..2a94466 100644
--- a/hw/xfree86/modes/xf86Crtc.c
+++ b/hw/xfree86/modes/xf86Crtc.c
@@ -2481,7 +2481,7 @@ xf86InitialConfiguration (ScrnInfoPtr scrn, Bool canGrow)
 
if (mode  crtc)
{
-   crtc-desiredMode = *mode;
+   xf86InternMode(crtc-desiredMode, mode);
crtc-desiredRotation = output-initial_rotation;
crtc-desiredX = output-initial_x;
crtc-desiredY = output-initial_y;
@@ -2663,7 +2663,7 @@ xf86SetDesiredModes (ScrnInfoPtr scrn)
 
if (!mode)
return FALSE;
-   crtc-desiredMode = *mode;
+   xf86InternMode(crtc-desiredMode, mode);
crtc-desiredRotation = RR_Rotate_0;
crtc-desiredTransformPresent = FALSE;
crtc-desiredX = 0;
@@ -2810,7 +2810,7 @@ xf86SetSingleMode (ScrnInfoPtr pScrn, DisplayModePtr 
desired, Rotation rotation)
ok = FALSE;
else
{
-   crtc-desiredMode = *crtc_mode;
+   xf86InternMode(crtc-desiredMode, crtc_mode);
crtc-desiredRotation = rotation;
crtc-desiredTransformPresent = FALSE;
crtc-desiredX = 0;
diff --git a/hw/xfree86/modes/xf86Modes.c b/hw/xfree86/modes/xf86Modes.c
index 49cc149..bc2fdf2 100644
--- a/hw/xfree86/modes/xf86Modes.c
+++ b/hw/xfree86/modes/xf86Modes.c
@@ -212,6 +212,17 @@ xf86DuplicateMode(const DisplayModeRec *pMode)
 }
 
 /**
+ * Fills in a copy of mode, removing all stale pointer references.
+ */
+void
+xf86InternMode(DisplayModeRec *intern, const DisplayModeRec *mode)
+{
+*intern = *mode;
+intern-prev = intern-next = NULL;
+intern-name = NULL;
+}
+
+/**
  * Duplicates every mode in the given list and returns a pointer to the first
  * mode.
  *
diff --git a/hw/xfree86/modes/xf86RandR12.c b/hw/xfree86/modes/xf86RandR12.c
index d5031a2..97dc35f 100644
--- a/hw/xfree86/modes/xf86RandR12.c
+++ b/hw/xfree86/modes/xf86RandR12.c
@@ -1236,7 +1236,7 @@ xf86RandR12CrtcSet (ScreenPtr pScreen,
/*
 * Save the last successful setting for

Re: [PATCH] xf86/modes: Defang stale pointers when copying modes

Re: [PATCH] xf86/modes: Defang stale pointers when copying modes

Re: [PATCH] xf86/modes: Defang stale pointers when copying modes

[PATCH] xf86/modes: Defang stale pointers when copying modes

[PATCH] xf86/modes: Defang stale pointers when copying modes

5 matches

Site Navigation

Mail list logo

Footer information