nxagentAdjustRandRXinerama uses freed memory

[vatral]

While debugging another issue, I stumbled across this one:

1. nxagentAdjustRandRXinerama (Screen.c:3808) allocates memory by calling RRGetInfo
2. nxagentAdjustRandRXinerama (Screen.c:3872) frees memory by calling FreeResource
3. nxagentAdjustRandRXinerama (Screen.c:3986) attempts to use the memory it just freed with RRCrtcSet

It looks like pScrPriv->outputs should be compacted to remove the deleted entries, or the entry should be marked as unused.

Issue found in cb8af80

Valgrind output:

==5904== Invalid write of size 4
==5904==    at 0x7F3204: RROutputChanged (rroutput.c:38)
==5904==    by 0x7ED3FF: RRCrtcNotify (rrcrtc.c:197)
==5904==    by 0x7ED910: RRCrtcSet (rrcrtc.c:670)
==5904==    by 0x4A7B5D: nxagentAdjustRandRXinerama (Screen.c:3986)
==5904==    by 0x4A7D47: nxagentChangeScreenConfig (Screen.c:3711)
==5904==    by 0x493998: nxagentHandleConfigureNotify (Events.c:3472)
==5904==    by 0x4957A7: nxagentDispatchEvents (Events.c:1949)
==5904==    by 0x49F4B6: nxagentBlockHandler (Handlers.c:458)
==5904==    by 0x45B3CC: BlockHandler (dixutils.c:440)
==5904==    by 0x46FEDF: WaitForSomething (WaitFor.c:262)
==5904==    by 0x4326DA: Dispatch (NXdispatch.c:362)
==5904==    by 0x40E61D: main (main.c:433)
==5904==  Address 0xb586c20 is 112 bytes inside a block of size 152 free'd
==5904==    at 0x4C2CD5A: free (vg_replace_malloc.c:530)
==5904==    by 0x7F31A3: RROutputDestroyResource (rroutput.c:414)
==5904==    by 0x448E2F: FreeResource (NXresource.c:320)
==5904==    by 0x4A76D7: nxagentAdjustRandRXinerama (Screen.c:3872)
==5904==    by 0x4A7D47: nxagentChangeScreenConfig (Screen.c:3711)
==5904==    by 0x493998: nxagentHandleConfigureNotify (Events.c:3472)
==5904==    by 0x4957A7: nxagentDispatchEvents (Events.c:1949)
==5904==    by 0x49F4B6: nxagentBlockHandler (Handlers.c:458)
==5904==    by 0x45B3CC: BlockHandler (dixutils.c:440)
==5904==    by 0x46FEDF: WaitForSomething (WaitFor.c:262)
==5904==    by 0x4326DA: Dispatch (NXdispatch.c:362)
==5904==    by 0x40E61D: main (main.c:433)
==5904==  Block was alloc'd at
==5904==    at 0x4C2BBAD: malloc (vg_replace_malloc.c:299)
==5904==    by 0x7F32C3: RROutputCreate (rroutput.c:92)
==5904==    by 0x7F0C80: RRScanOldConfig (rrinfo.c:100)
==5904==    by 0x7F0C80: RRGetInfo (rrinfo.c:206)
==5904==    by 0x4A74DF: nxagentAdjustRandRXinerama (Screen.c:3808)
==5904==    by 0x4A7D47: nxagentChangeScreenConfig (Screen.c:3711)
==5904==    by 0x493998: nxagentHandleConfigureNotify (Events.c:3472)
==5904==    by 0x4957A7: nxagentDispatchEvents (Events.c:1949)
==5904==    by 0x49F4B6: nxagentBlockHandler (Handlers.c:458)
==5904==    by 0x45B3CC: BlockHandler (dixutils.c:440)
==5904==    by 0x46FEDF: WaitForSomething (WaitFor.c:262)
==5904==    by 0x4326DA: Dispatch (NXdispatch.c:362)
==5904==    by 0x40E61D: main (main.c:433)

[uli42]

While developing this I had the problem that the original xrandr code was missing a free() call. So I had to work around that somehow. The missing free() has been backported IIRC. So it could be that the workaround is wrong now.

[vatral]

It does look like something needs fixing.

FreeResource is a free() call, and the code that calls it doesn't decrement pScrPriv->numOutputs, so that entry is still in the array, and pointing to freed memory. It would seem the logical thing to do is to either compact the array after deletion, or to set the entry to NULL to mark it as unused.

I've not yet looked at what is the right thing to do there, and may be busy with other things tomorrow, so it would be good if somebody else could take a look at it.

[sunweaver]

[mike@minobo nxagent (3.6.x)]$ git diff
diff --git a/nx-X11/programs/Xserver/hw/nxagent/Screen.c b/nx-X11/programs/Xserver/hw/nxagent/Screen.c
index e645129..9aa7dca 100644
--- a/nx-X11/programs/Xserver/hw/nxagent/Screen.c
+++ b/nx-X11/programs/Xserver/hw/nxagent/Screen.c
@@ -3863,16 +3863,6 @@ int nxagentAdjustRandRXinerama(ScreenPtr pScreen)
       }
     }
 
-    /* delete superfluous non-NX outputs */
-    for (i = pScrPriv->numOutputs - 1; i >= 0; i--) {
-      if (strncmp(pScrPriv->outputs[i]->name, "NX", 2)) {
-        #ifdef DEBUG
-        fprintf(stderr, "nxagentAdjustRandRXinerama: destroying output [%s]\n", pScrPriv->outputs[i]->name);
-        #endif
-        RROutputDestroy(pScrPriv->outputs[i]);
-      }
-    }
-
     /* at this stage only NX outputs are left - we delete the superfluous ones */
     for (i = pScrPriv->numOutputs - 1; i >= number; i--) {
       #ifdef DEBUG

To me it seems we should / could simply drop the above code fraction.

@uli42: @vatral: ?

[sunweaver]

Oh hang on... wrong section removed, I just realized. However, that above passage does not make sense either, does it? Where should non-NX Outputs come from?

[sunweaver]

more noise... the above passage was the correct code block to be removed...

[sunweaver]

@uli42: @vatral: ?

[vatral]

@sunweaver Hrmm. This is confusing. Let's see if I understand.

That chunk of code removes unneeded outputs. Note that there's another one right after it, so if you're going to remove something, it's probably both.

The code iterates from the end of the list, because RROutputDestroy actually compacts the pScrPriv->outputs array.

Lower down, the problem happens with "RRCrtcSet(pScrPriv->crtcs[i], ...", because RRCrtcSet ends up calling RROutputChanged on a freed RROutput. Also note this:

for (i = 0; i numOutputs; i++ ) {
    RROutputSetModes(pScrPriv->outputs[i], NULL, 0, 0);
    ...
    RRCrtcSet(pScrPriv->crtcs[i], NULL, 0, 0, RR_Rotate_0, 1, 

So the assumption there seems to be that pScrPriv->outputs and pScrPriv->crtcs have a 1 to 1 relationship, but that seems to be false, because when we delete from "outputs" in RROutputDestroy we are not touching crtcs.

Then there's that a RROutput has both a .crtc member and a .crtcs array. So far I see:

• A Screen has RROutputs
• An RROutput has a list of RRCrtcs
• Destroying a RROutput destroys the list but not the RRCrtcs themselves, so something else owns those objects
• An RRCrtc has a list of RROutputs inside it

Ok, now I'm confused about what is the hierarchy of this and who owns what. Are pScrPriv->outputs and pScrPriv->crtcs supposed to go in sync? Should deleting a RROutput reach into RRCrtcs and remove any references it holds to the deleted object?

[uli42]

Well, concerning the NX outputs this 1:1 relationship is true, IIRC. If an output on the real xserver is not visible inside NX it gets switched off by removing the crtc. You can comment some of the cleanup routines and check what the servers knows using the xrandr binary. Maybe it gets clearer what's going on. I need to look deeper into this but I am lacking time currently, sorry.

[uli42]

Maybe the spec helps for better understanding of the output<->crtc relationship: https://cgit.freedesktop.org/xorg/proto/randrproto/tree/randrproto.txt (graphical representation around line 80)

The 1:1 relationship is true in NX as long as noone is adding additional outputs using the RAND extension. Which is something that should be prevented anyway, I'd say.

[opoplawski]

Just stumbled across this as well testing 3.5.99.3

[uli42]

it looks like pScrPriv->outputs should be compacted to remove the deleted entries, or the entry should be marked as unused.

After looking through the code I think this remark is correct.

Update:
Some code doing this can be found in rroutput.c/RROutputDestroyResource(void *value, XID pid):

        for (i = 0; i < pScrPriv->numOutputs; i++) {
            if (pScrPriv->outputs[i] == output) {
                memmove(pScrPriv->outputs + i, pScrPriv->outputs + i + 1,
                        (pScrPriv->numOutputs - (i + 1)) * sizeof(RROutputPtr));
                --pScrPriv->numOutputs;
                break;
            }
        }

If everything goes right RROutputDestroyResource() should be called automatically since it is used as the deleteFunc for the Output resource type:

Bool
RROutputInit(void)
{
    RROutputType = CreateNewResourceType(RROutputDestroyResource
#ifndef NXAGENT_SERVER
                                         , "OUTPUT"
#endif
        );

So in theory the postulated compaction of the output array is already done.

[uli42]

I have confirmed that RROutputDestroyResource() gets executed when FreeResource() is called for an output. So the output array gets compacted if necessary.

Update: we could have saved all this as RROutputDestroyResource happens to occur in the valgrind output above...

[uli42]

Now, this is interesting:

==5904==    by 0x7F31A3: RROutputDestroyResource (rroutput.c:414)
==5904==    by 0x448E2F: FreeResource (NXresource.c:320)
==5904==    by 0x4A76D7: nxagentAdjustRandRXinerama (Screen.c:3872)

If I interpret valgrind's output correctly this tells me that nxagentAdjustRandRXinerama() calls FreeResource() which in turn calls RROutputDestroyResource(). So we can conclude that FreeResource() is used here to free an Output resource. However, the code contains only one FreeResource() call, and that one is freeing a mode, not an Output!

      if (prevmode && prevmode->refcnt == 1) {
        #ifdef DEBUG
        fprintf(stderr, "nxagentAdjustRandRXinerama: destroying prevmode [%s]\n", prevmode->name);
        #endif
	FreeResource(prevmode->mode.id, 0);
      }

So the question is: why do we end up in RROutputDestroyResource() when freeing a mode resource?

[uli42]

Update2: ignore this comment, it's nonsense. See further comments.

Well, while your commit seems to make the behaviour disappear I think I have taken a step towards the real cause: During writing Xinerama stuff I have added nxagentRandRCrtcSet() (in Extensions.c). This simply calls RRCrtcNotify(). I remember that this was the easiest approach to get the desired behaviour though I had been unsure about it. Not without a reason, it seems...

RRCrtcNotify has this comment:

 * Notify the extension that the Crtc has been reconfigured,
 * the driver calls this whenever it has updated the mode

We have not changed the mode here but the number of outputs (by destroying one). So it is probably not the right function to call.

I think nxagentRandRCrtcSet() should be reimplemented to do the right thing itself.

(Proof of concept: changing nxagentRandRCrtcSet to just return TRUE makes the issue disappear. Must check if xinerama is still working, though)

Update: http://xorg-devel.x.narkive.com/KpOwRcMX/patch-xvfb-add-randr-support also calls RRCrtcNotify here. Hmm...

[sunweaver]

Hi Uli,

On Mo 06 Feb 2017 23:26:34 CET, Ulrich Sibiller wrote: Well, while your commit seems to make the behaviour disappear I think I have taken a step towards the real cause. During writing Xinerama stuff I have added nxagentRandRCrtcSet() (in Extensions.c). This simply calls RRCrtcNotify(). I remember that this was the easiest approach to get the desired behaviour though I had been unsure about it. It seems by feelings back then were right. I think nxagentRandRCrtcSet() should be reimplemented to do the right thing itself.

What exactly to you be by "to do the right thing"? Mike -- DAS-NETZWERKTEAM mike gabriel, herweg 7, 24357 fleckeby mobile: +49 (1520) 1976 148 landline: +49 (4354) 8390 139 GnuPG Fingerprint: 9BFB AEE8 6C0A A5FF BF22 0782 9AF4 6B30 2577 1B31 mail: mike.gabriel@das-netzwerkteam.de, http://das-netzwerkteam.de

[uli42]

I think "the right thing" is to do what RRCrtcNotify() does but without touch old stuff. The real problem is that the old output (that has been destroyed just before) is still referenced in the crtc struct and RRNotify() will try to set the "changed" flag on it.

It could also be that calling RROutputDestroy() it not sufficient and some more work is needed after it. I need to investigate but I am lacking time...

[uli42]

The real reason is identified: The "delete superfluous non-NX outputs" loop adjusts the number of outputs to the number of crtcs; both should match the number of outputs of the real server. It does that by calling RROutputDestroy() for all the outputs in the outputs array whose index exceeds the required number. But it does not check if any of these outputs is still in use by a crtc. So a crtc can have reference to a destroyed output. And on RRCrtcNotify() that output reference is found and the output is marked as changed by setting the output's "changed" flag. As the output has already been destroyed and its memory has been freed valgrind complains (which is of course correct).

What does the proper fix look like? I am not sure. Maybe first try to identify unused outputs and first destroy them. But what to do if they are not enough? Can that happen at all?

Currently the outputs are deleted from the highest index to the lowest index in the outputs array. Simply reversing the order will probably help for exactly this situation where a seeing here but it feels wrong as a fix.

This memory access happens first for the "default" output.