Thomas Hellstrom <[email protected]> writes: > Hi! > > I'd like to draw your attention to bug 106960 where the new leases code > accesses freed memory.
Thanks. I've got a proposed fix which re-works where leases are taken down during X server reset or termination. > On xf86-video-vmware it causes a server segfault. On modesetting it > doesn't (yet) but can be seen with valgrind. I've hacked up a client to take a lease and close the X connection so that the lease hangs around for X server shutdown for testing; the results were pretty spectacular. The leases held pointers to crtcs and outputs; during termination, the crtcs and outputs were freed when their associated XIDs were freed, which happens before CloseScreen. At CloseScreen time, the leases still held pointers to those objects which caused all sorts of issues. The patch below terminates any lease referencing a CRTC or output that is being destroyed; that makes the leases go away before CloseScreen. Any remaining leases (which must have no outputs or CRTCs at all) get terminated in RRCloseScreen. Lease termination is no longer necessary in the driver now, so I've removed it from the modesetting driver. I also discovered that leasing is broken with current X server master; patches to fix that have been sent to the list under a separate cover.
From 509d59ce4f9faa939c83a987aae7552293c8b624 Mon Sep 17 00:00:00 2001 From: Keith Packard <[email protected]> Date: Tue, 26 Jun 2018 09:20:00 -0700 Subject: [PATCH xserver] During reset/shutdown, clean up leases in DIX instead of each driver Instead of having every video driver loop over any pending leases to free them during CloseScreen, do this up in the DIX layer by terminating leases when a leased CRTC or Output is destroyed and (just to make sure), also terminating leases in RRCloseScreen. The latter should "never" get invoked as any lease should be associated with a resource which was destroyed. This is required as by the time the driver's CloseScreen function is invoked, we've already freed all of the DIX randr structures and no longer have any way to reference the leases Signed-off-by: Keith Packard <[email protected]> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=106960 Cc: Thomas Hellstrom <[email protected]> --- hw/xfree86/drivers/modesetting/driver.c | 2 -- .../drivers/modesetting/drmmode_display.c | 17 ----------------- .../drivers/modesetting/drmmode_display.h | 2 -- randr/randr.c | 4 ++++ randr/randrstr.h | 3 +++ randr/rrcrtc.c | 11 +++++++++++ randr/rrlease.c | 2 +- randr/rroutput.c | 11 +++++++++++ 8 files changed, 30 insertions(+), 22 deletions(-) diff --git a/hw/xfree86/drivers/modesetting/driver.c b/hw/xfree86/drivers/modesetting/driver.c index 66ed2b811..02cd0b4ac 100644 --- a/hw/xfree86/drivers/modesetting/driver.c +++ b/hw/xfree86/drivers/modesetting/driver.c @@ -1812,8 +1812,6 @@ CloseScreen(ScreenPtr pScreen) ms->drmmode.shadow_fb2 = NULL; } - drmmode_terminate_leases(pScrn, &ms->drmmode); - drmmode_uevent_fini(pScrn, &ms->drmmode); drmmode_free_bos(pScrn, &ms->drmmode); diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c index dbb885e8e..fa0a46dc3 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.c +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c @@ -3325,23 +3325,6 @@ drmmode_terminate_lease(RRLeasePtr lease) } } -void -drmmode_terminate_leases(ScrnInfoPtr pScrn, drmmode_ptr drmmode) -{ - ScreenPtr screen = xf86ScrnToScreen(pScrn); - rrScrPrivPtr scr_priv = rrGetScrPriv(screen); - RRLeasePtr lease, next; - - xorg_list_for_each_entry_safe(lease, next, &scr_priv->leases, list) { - drmmode_lease_private_ptr lease_private = lease->devPrivate; - drmModeRevokeLease(drmmode->fd, lease_private->lessee_id); - free(lease_private); - lease->devPrivate = NULL; - RRLeaseTerminated(lease); - RRLeaseFree(lease); - } -} - static const xf86CrtcConfigFuncsRec drmmode_xf86crtc_config_funcs = { .resize = drmmode_xf86crtc_resize, .create_lease = drmmode_create_lease, diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.h b/hw/xfree86/drivers/modesetting/drmmode_display.h index e46a292d3..cde661450 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.h +++ b/hw/xfree86/drivers/modesetting/drmmode_display.h @@ -272,8 +272,6 @@ extern Bool drmmode_setup_colormap(ScreenPtr pScreen, ScrnInfoPtr pScrn); extern void drmmode_uevent_init(ScrnInfoPtr scrn, drmmode_ptr drmmode); extern void drmmode_uevent_fini(ScrnInfoPtr scrn, drmmode_ptr drmmode); -extern void drmmode_terminate_leases(ScrnInfoPtr scrn, drmmode_ptr drmmode); - Bool drmmode_create_initial_bos(ScrnInfoPtr pScrn, drmmode_ptr drmmode); void *drmmode_map_front_bo(drmmode_ptr drmmode); Bool drmmode_map_cursor_bos(ScrnInfoPtr pScrn, drmmode_ptr drmmode); diff --git a/randr/randr.c b/randr/randr.c index feb54bcc8..5db8b5ced 100644 --- a/randr/randr.c +++ b/randr/randr.c @@ -89,8 +89,12 @@ RRCloseScreen(ScreenPtr pScreen) { rrScrPriv(pScreen); int j; + RRLeasePtr lease, next; unwrap(pScrPriv, pScreen, CloseScreen); + + xorg_list_for_each_entry_safe(lease, next, &pScrPriv->leases, list) + RRTerminateLease(lease); for (j = pScrPriv->numCrtcs - 1; j >= 0; j--) RRCrtcDestroy(pScrPriv->crtcs[j]); for (j = pScrPriv->numOutputs - 1; j >= 0; j--) diff --git a/randr/randrstr.h b/randr/randrstr.h index f94174b54..2cede92e3 100644 --- a/randr/randrstr.h +++ b/randr/randrstr.h @@ -829,6 +829,9 @@ RRCrtcIsLeased(RRCrtcPtr crtc); extern _X_EXPORT Bool RROutputIsLeased(RROutputPtr output); +void +RRTerminateLease(RRLeasePtr lease); + Bool RRLeaseInit(void); diff --git a/randr/rrcrtc.c b/randr/rrcrtc.c index d7d937e80..d31e9663f 100644 --- a/randr/rrcrtc.c +++ b/randr/rrcrtc.c @@ -871,6 +871,17 @@ RRCrtcDestroyResource(void *value, XID pid) if (pScreen) { rrScrPriv(pScreen); int i; + RRLeasePtr lease, next; + + xorg_list_for_each_entry_safe(lease, next, &pScrPriv->leases, list) { + int c; + for (c = 0; c < lease->numCrtcs; c++) { + if (lease->crtcs[c] == crtc) { + RRTerminateLease(lease); + break; + } + } + } for (i = 0; i < pScrPriv->numCrtcs; i++) { if (pScrPriv->crtcs[i] == crtc) { diff --git a/randr/rrlease.c b/randr/rrlease.c index b4a8827cc..e25d9ca99 100644 --- a/randr/rrlease.c +++ b/randr/rrlease.c @@ -169,7 +169,7 @@ RRLeaseFree(RRLeasePtr lease) * finished, which may be some time after this function returns * if the driver operation is asynchronous */ -static void +void RRTerminateLease(RRLeasePtr lease) { ScreenPtr screen = lease->screen; diff --git a/randr/rroutput.c b/randr/rroutput.c index 33300e1cc..e52ad7671 100644 --- a/randr/rroutput.c +++ b/randr/rroutput.c @@ -379,6 +379,17 @@ RROutputDestroyResource(void *value, XID pid) if (pScreen) { rrScrPriv(pScreen); int i; + RRLeasePtr lease, next; + + xorg_list_for_each_entry_safe(lease, next, &pScrPriv->leases, list) { + int o; + for (o = 0; o < lease->numOutputs; o++) { + if (lease->outputs[o] == output) { + RRTerminateLease(lease); + break; + } + } + } if (pScrPriv->primaryOutput == output) pScrPriv->primaryOutput = NULL; -- 2.17.1
-- -keith
signature.asc
Description: PGP signature
_______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
