Bug 4247

Summary:

Access to freed memory in argb cursor code on server exit

Product:

xorg

Reporter:

Matthieu Herrb <matthieu.herrb>

Component:

Server/General

Assignee:

Xorg Project Team <xorg-team>

Status:

RESOLVED FIXED

QA Contact:

Severity:

normal

Priority:

high

CC:

eich, keithp, sndirsch

Version:

git

Keywords:

patch

Hardware:

x86 (IA32)

OS:

OpenBSD

Whiteboard:

i915 platform:

i915 features:

Bug Depends on:

Bug Blocks:

5799, 6666

Attachments:

Description	Flags
patch to trace the problem	none
This is a patch implementing the workaround I propose.	ajax: 6.9/7.0?

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 
Xprint/dpmsstubs.o 
+os/libcwrapper.o dix/libdix.a os/libos.a  Xprint/libprinter.a      
Xprint/raster/libraster.a 
+Xprint/pcl/libpcl.a Xprint/pcl-mono/libpcl.a Xprint/ps/libps.a mfb/libmfb.a 
cfb/libcfb.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                   
../../lib/lbxutil/liblbxutil.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  
-Wl,-rpath-link,../../exports/lib 
Xprint/ps/libps.a(psout_ftpstype1.o): In function `PsOut_DownloadFreeType1': 
/usr/src/packages/BUILD/xc/programs/Xserver/Xprint/ps/psout_ftpstype1.c:77: 
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 
+`miDCIsRootPicture' 
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

Thanks!

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.