xf86: take next pointer before calling handler

Submitted by Maarten Lankhorst on Oct. 8, 2012, 10:40 a.m.

Details

Message ID 5072AD98.5020305@canonical.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Maarten Lankhorst Oct. 8, 2012, 10:40 a.m. 
stopping acpid before Xorg with valgrind --free-fill=df results in this crash backtrace:

==2670== Invalid read of size 8
==2670==    at 0x1B9CB0: xf86Wakeup (xf86Events.c:276)
==2670==    by 0x1687B2: WakeupHandler (dixutils.c:423)
==2670==    by 0x334793: WaitForSomething (WaitFor.c:224)
==2670==    by 0x159E4B: Dispatch (dispatch.c:357)
==2670==    by 0x14AA78: main (main.c:295)
==2670==  Address 0x783aa90 is 32 bytes inside a block of size 40 free'd
==2670==    at 0x4C2A739: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2670==    by 0x1BA8CA: removeInputHandler (xf86Events.c:645)
==2670==    by 0x1BA96D: xf86RemoveGeneralHandler (xf86Events.c:681)
==2670==    by 0x1F106C: lnxCloseACPI (lnx_acpi.c:174)
==2670==    by 0x1F0CC7: lnxACPIGetEventFromOs (lnx_acpi.c:68)
==2670==    by 0x1CCF49: xf86HandlePMEvents (xf86PM.c:208)
==2670==    by 0x1B9CAB: xf86Wakeup (xf86Events.c:279)
==2670==    by 0x1687B2: WakeupHandler (dixutils.c:423)
==2670==    by 0x334793: WaitForSomething (WaitFor.c:224)
==2670==    by 0x159E4B: Dispatch (dispatch.c:357)
==2670==    by 0x14AA78: main (main.c:295)
==2670== 
==2670== Invalid read of size 4
==2670==    at 0x1B9C29: xf86Wakeup (xf86Events.c:277)
==2670==    by 0x1687B2: WakeupHandler (dixutils.c:423)
==2670==    by 0x334793: WaitForSomething (WaitFor.c:224)
==2670==    by 0x159E4B: Dispatch (dispatch.c:357)
==2670==    by 0x14AA78: main (main.c:295)
==2670==  Address 0xdfdfdfdfdfdfdff7 is not stack'd, malloc'd or (recently) free'd
==2670== 
(EE)==2670== 
==2670== Process terminating with default action of signal 11 (SIGSEGV): dumping core

Taking a pointer to ih->next before calling the event handler fixes this.

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

---

Patch hide | download patch | download mbox 

diff --git a/hw/xfree86/common/xf86Events.c b/hw/xfree86/common/xf86Events.c
index 3ad34b5..98ee8cd 100644
--- a/hw/xfree86/common/xf86Events.c
+++ b/hw/xfree86/common/xf86Events.c
@@ -271,9 +271,10 @@  xf86Wakeup(pointer blockData, int err, pointer pReadmask)
     }
 
     if (err >= 0) {             /* we don't want the handlers called if select() */
-        IHPtr ih;               /* returned with an error condition, do we?      */
+        IHPtr ih, ih_next;      /* returned with an error condition, do we?      */
 
-        for (ih = InputHandlers; ih; ih = ih->next) {
+        for (ih = InputHandlers; ih; ih = ih_next) {
+            ih_next = ih->next;
             if (ih->enabled && ih->fd >= 0 && ih->ihproc &&
                 (FD_ISSET(ih->fd, ((fd_set *) pReadmask)) != 0)) {
                 ih->ihproc(ih->fd, ih->data);

Comments

Keith Packard Oct. 8, 2012, 5:54 p.m. 
Maarten Lankhorst <maarten.lankhorst@canonical.com> writes:

> Taking a pointer to ih->next before calling the event handler fixes
> this.

Yay, more open coded lists. Any way we could encourage you to replace it
with a struct xorg_list instead?
Aaron Plattner Oct. 8, 2012, 8:13 p.m. 
On 10/08/2012 10:54 AM, Keith Packard wrote:
> * PGP Signed by an unknown key
>
> Maarten Lankhorst <maarten.lankhorst@canonical.com> writes:
>
>> Taking a pointer to ih->next before calling the event handler fixes
>> this.
>
> Yay, more open coded lists. Any way we could encourage you to replace it
> with a struct xorg_list instead?

Or at least

   nt_list_for_each_entry_safe(ih, ih_tmp, InputHandlers, next) {
       ...
   }

if you need a short-term fix.

-- Aaron