Compiz crashes when restoring decoration to a window

Created by: alekseyt

Steps to reproduce:

1. Open ccsm -> Window Decoration
2. Set the following rule to Decoration Windows: !(state=maxvert & state=maxhorz)

Now all maximized windows are not decorated.

3. Maximize a window
4. Unmaximize a window

Result: compiz crashes.

Alternative steps to reproduce:

1. Open ccsm -> Window Decoration
2. Decoration Windows -> change any to !(ccsm window id) (you can pick id with picker) and press tab

Now ccsm window is not decorated.

3. Change Decoration Windows back to any

Now ccsm window is decorated again.

4. Repeat 2-3 until compiz crashes

In this scenario it's better to start from not decorated ccsm window, then change window id to any, then back to window id, then back to any, here compiz usually always crashes.

In my debugging attempt i've found that it crashes with the following stack trace (valgrind output):

==19839== Use of uninitialised value of size 8
==19839==    at 0x4C31153: memcpy@GLIBC_2.2.5 (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==19839==    by 0xEF46C6C: ??? (in /usr/lib/xorg/modules/drivers/swrast_dri.so)
==19839==    by 0xEF449C9: ??? (in /usr/lib/xorg/modules/drivers/swrast_dri.so)
==19839==    by 0x117C67: enableTexture (texture.c:423)
==19839==    by 0x141E37: enableFragmentOperationsAndDrawGeometry (paint.c:1115)
==19839==    by 0x142094: drawWindowTexture (paint.c:1183)
==19839==    by 0x177823D3: decorDrawWindow (decoration.c:239)
==19839==    by 0x14236B: paintWindow (paint.c:1267)
==19839==    by 0x16D6B8EC: movePaintWindow (move.c:871)
==19839==    by 0x16F73CA5: resizePaintWindow (resize.c:1469)
==19839==    by 0x13EE5C: paintOutputRegion (paint.c:412)
==19839==    by 0x13F5D2: paintOutput (paint.c:570)
==19839== 
==19839== Invalid write of size 2
==19839==    at 0x4C31153: memcpy@GLIBC_2.2.5 (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==19839==    by 0xEF46C6C: ??? (in /usr/lib/xorg/modules/drivers/swrast_dri.so)
==19839==    by 0xEF449C9: ??? (in /usr/lib/xorg/modules/drivers/swrast_dri.so)
==19839==    by 0x117C67: enableTexture (texture.c:423)
==19839==    by 0x141E37: enableFragmentOperationsAndDrawGeometry (paint.c:1115)
==19839==    by 0x142094: drawWindowTexture (paint.c:1183)
==19839==    by 0x177823D3: decorDrawWindow (decoration.c:239)
==19839==    by 0x14236B: paintWindow (paint.c:1267)
==19839==    by 0x16D6B8EC: movePaintWindow (move.c:871)
==19839==    by 0x16F73CA5: resizePaintWindow (resize.c:1469)
==19839==    by 0x13EE5C: paintOutputRegion (paint.c:412)
==19839==    by 0x13F5D2: paintOutput (paint.c:570)
==19839==  Address 0x118c93440 is not stack'd, malloc'd or (recently) free'd

It's this code in texture.c:

	(*screen->bindTexImage) (screen->display->display,
				 texture->pixmap,
				 GLX_FRONT_LEFT_EXT,
				 NULL);

I've checked if texture was free'd before or pixmap was free'd before - as far as i can see they weren't. I've also checked reference counting on textures - seems ok to me. enableTexture() also used in many places of compiz, so i assumed it's somehow GL issue, not necessarily C++ issue.

My assumption is that decoration is using some outdated pixbufs/textures and copy from pixbuf to texture fails, or vise versa. Then it's probably reusing outdated pixbuf/texture and crashes. I've made the following change: when window is undecorated, its decorations released completely, not only WindowDecoration struct, but also Decoration.

Now, i'm not really expert in compiz, decoration.c is also kind of lacking in comments, but here is patch:

From 8bc5a6a1f331d143e47374e23b53317cb7cd0cd6 Mon Sep 17 00:00:00 2001
From: Aleksey Tulinov <aleksey.tulinov@gmail.com>
Date: Fri, 26 Jan 2018 10:54:51 +0200
Subject: [PATCH] decoration: Fixed crash when re-enabling decoration on a
 window where decoration was previously disabled

---
 plugins/decoration.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/plugins/decoration.c b/plugins/decoration.c
index e2068926..4f09150a 100644
--- a/plugins/decoration.c
+++ b/plugins/decoration.c
@@ -367,7 +367,7 @@ decorReleaseTexture (CompScreen   *screen,
     DECOR_DISPLAY (screen->display);
 
     texture->refCount--;
-    if (texture->refCount)
+    if (texture->refCount > 0)
 	return;
 
     if (texture == dd->textures)
@@ -658,9 +658,11 @@ decorUpdateDecorations (CompScreen   *screen,
        if the decoration changed, and /then/ release
        and re-update it, but for now just releasing all */
     if (decors && *decorNum > 0)
-	decorReleaseDecorations (screen, decors, decorNum);
+    {
+    decorReleaseDecorations (screen, decors, decorNum);
     decors = NULL;
     *decorNum = 0;
+    }
 
     result = XGetWindowProperty (screen->display->display, id,
 				 decorAtom, 0L,
@@ -737,7 +739,7 @@ decorUpdateDecorations (CompScreen   *screen,
 	{
 	    unsigned int realDecorNum = i;
 	    decorReleaseDecorations (screen, decors, &realDecorNum);
-	    *decorNum = realDecorNum;
+	    *decorNum = 0;
 	    XFree (data);
 	    return NULL;
 	}
@@ -1333,8 +1335,15 @@ decorWindowUpdate (CompWindow *w,
 	memset (&emptyInput, 0, sizeof (emptyInput));
 	setWindowFrameExtents (w, &emptyInput);
 
+	/* decoration was already destroyed under if (old) */
 	dw->wd = NULL;
 
+	if (dw->decors && dw->decorNum > 0)
+	decorReleaseDecorations (w->screen, dw->decors, &dw->decorNum);
+
+	dw->decors = NULL;
+	dw->decorNum = 0;
+
 	moveDx = -oldShiftX;
 	moveDy = -oldShiftY;
 
-- 
2.15.1

Patch includes some other fixes i've made while debugging to make code more correct. Important part is at bottom:

+	if (dw->decors && dw->decorNum > 0)
+	decorReleaseDecorations (w->screen, dw->decors, &dw->decorNum);

If fix is ok in general, i can reformat the patch and remove "unneeded" bits or make a pull request (but i would prefer to send a patch instead of pull request).