Bug 4247 - Access to freed memory in argb cursor code on server exit
Summary: Access to freed memory in argb cursor code on server exit
Alias: None
Product: xorg
Classification: Unclassified
Component: Server/General (show other bugs)
Version: git
Hardware: x86 (IA32) OpenBSD
: high normal
Assignee: Xorg Project Team
QA Contact:
Keywords: patch
Depends on:
Blocks: 5799 xorg-7.2
  Show dependency treegraph
Reported: 2005-08-25 15:16 UTC by Matthieu Herrb
Modified: 2006-05-25 02:58 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:

patch to trace the problem (2.05 KB, patch)
2005-08-25 15:22 UTC, Matthieu Herrb
no flags Details | Splinter Review
This is a patch implementing the workaround I propose. (584 bytes, patch)
2005-09-03 09:10 UTC, Matthieu Herrb
ajax: 6.9/7.0?
Details | Splinter Review

Description Matthieu Herrb 2005-08-25 15:16:34 UTC
Yet another bug revelead by OpenBSD's malloc(). 
When the server quits, miDCCloseScreen() calls the destroyPixmap on
pScreenPriv->pRootPicturs. But this pixmap has already been freed by
PictureDestroyWindow() which was iterates over all Pictures owned by a window.

The desproPixmap method tries to access the refcount field in the pixmap structure 
and causes a segmentation fault when the freed memory is not readable anymore. 
The easiest fix may be not to free this pixmap in miDCCloseScreen anymore. 

I'm attaching a patch that adds some traces to show the problem, even on systems
that won't cause a segfault when accessing free memory.
Comment 1 Matthieu Herrb 2005-08-25 15:22:24 UTC
Created attachment 3035 [details] [review]
patch to trace the problem

observe the following sequence in the X server output:

pRootPicture: 0x80fe0f00 1
Freeing RootPicture		 <- this happens in PictureDestroyWindow()
tossPict 0x80fe0f00 -791621424	 <- and this in miDCCloseScreen() 
				    pRootPicture points to free memory.
Comment 2 Egbert Eich 2005-09-01 02:17:50 UTC
This pixmap is represented in two locations. Therefore it's not easy to let the
second call to free know that other location has already done the free.
I agree with your suggested fix but we need to know if we may not run into
situations where PictureDestroyWindow() doesn't get called for this window (I
assume it's thre root window).
Keith would probably know best - adding him to Cc:
Comment 3 Stefan Dirsch 2005-09-03 08:59:19 UTC
Unfortunately the patch breaks the build of Xprt (Xorg server build works): 
gcc -o Xprt -O2 -fmessage-length=0 -Wall  -g -fno-strict-aliasing -ansi 
-pedantic -Wall -Wpointer-arith 
+-Wundef     -L../../exports/lib   Xprint/ddxInit.o Xprint/miinitext.o 
+os/libcwrapper.o dix/libdix.a os/libos.a  Xprint/libprinter.a      
+Xprint/pcl/libpcl.a Xprint/pcl-mono/libpcl.a Xprint/ps/libps.a mfb/libmfb.a 
+cfb32/libcfb32.a mfb/libmfb.a dix/libxpstubs.a mi/libmi.a 
composite/libcomposite.a damageext/libdamage.a 
+miext/damage/libdamage.a xfixes/libxfixes.a miext/cw/libcw.a             
Xext/libexts.a xkb/libxkb.a 
+Xi/libxinput.a                   lbx/liblbx.a                   
+randr/librandr.a render/librender.a Xext/libext.a dbe/libdbe.a 
record/librecord.a GL/glx/libglx.a 
+GL/mesa/GLcore/libGLcore.a XTrap/libxtrap.a     ../../lib/font/libXfont.a 
-lfreetype dix/libxpstubs.a 
+-lz -lm   -lusb -lresmgr                   -lXau -lXdmcp  
Xprint/ps/libps.a(psout_ftpstype1.o): In function `PsOut_DownloadFreeType1': 
warning: the use of `tempnam' 
+is dangerous, better use `mkstemp' 
render/librender.a(picture.o): In function `PictureDestroyWindow': 
/usr/src/packages/BUILD/xc/programs/Xserver/render/picture.c:116: undefined 
reference to 
collect2: ld returned 1 exit status 
make[4]: *** [Xprt] Error 1 
Comment 4 Matthieu Herrb 2005-09-03 09:09:22 UTC
The patch is only there to hilight the problem. It's not a fix, so I didn't
consider breaking Xprt as a big issue. 
Comment 5 Matthieu Herrb 2005-09-03 09:10:49 UTC
Created attachment 3162 [details] [review]
This is a patch implementing the workaround I propose.
Comment 6 Stefan Dirsch 2005-09-03 09:12:57 UTC
Comment 7 Adam Jackson 2005-11-30 16:52:51 UTC
Comment on attachment 3162 [details] [review]
This is a patch implementing the workaround I propose. 

looks sane, worst it could do is leak.
Comment 8 Adam Jackson 2006-05-16 23:39:36 UTC
keith, correctness comments please.
Comment 9 Keith Packard 2006-05-17 05:07:42 UTC
Yeah, the patch looks correct; all pictures pointing at windows needn't get
freed as they are freed when the window is destroyed.
Comment 10 Adam Jackson 2006-05-19 03:31:25 UTC
-> 7.2
Comment 11 Matthieu Herrb 2006-05-25 19:58:28 UTC
Committed now.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.