Bug 4168

Summary:

access to freed memory in the modules loader

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:

dberkholz, erik.andren

Version:

git

Keywords:

patch

Hardware:

All

OS:

All

Whiteboard:

i915 platform:

i915 features:

Bug Depends on:

Bug Blocks:

5387, 5799

Attachments:

Description	Flags
Proposed patch	ajax: 6.9/7.0?

Description Matthieu Herrb 2005-08-21 02:19:41 UTC

Using OpenBSD's new memory allocator which is a lot less tolerant
regarding accesses to unallocated memory than others, people have
started reporting failure to start the X server whithout a xorg.conf
configuration file. Mark Kettenis provided a detailled analysis of the
problem. 

Modules add references to symbols they need to a global list using the
LoaderRefSymList() function. This function calls AppendSymbol(), which
stores a pointer to the string in the module's address space into the
list. 

When a module is unloaded (which happens when autoconfiguration is
used), the strings in the module's address space are no longer
available, but the refList list still references them. Any further
iteration on this list will cause access to unallocated memory.

Since the loader does not provide a mean to clean up list entries when
unloading a module, I think that the best fix for this problem is to
copy the strings into the list, as done in the attached patch.

Comment 1 Matthieu Herrb 2005-08-21 02:20:14 UTC

Created attachment 2959 [details] [review]
Proposed patch

Comment 2 Matthieu Herrb 2005-08-28 13:39:17 UTC

Comment on attachment 2959 [details] [review]
Proposed patch

Oops I did not mean to edit this patch

Comment 3 Egbert Eich 2005-09-01 03:33:05 UTC

The fact that symbols cannot get nuked when the module is unloaded is a design
flaw. I can imagine that if a symbol has been marked required by a module that
has then be nuked will generate bogus errors because it may be unresolved in
another driver which however doesn't need it. The reason is that if one driver is 
unloaded all its submodules are nuked if their refcount is 0.
The right fix would involve to add an argument to the LoaderRefSymbols() and
LoaderReqSymbols() function and their SymList equivalents so that the module
adding it could be identified.
We at least should avoid copying the same string multiple times.
This will add bw compatibility issues - but maybe that's what needs to be done.

Comment 4 Adam Jackson 2005-11-19 04:29:49 UTC

of course, dlloader makes LoaderRefSymList redundant anyway.

the only issue i have with this patch is that it leaks, the xnfalloc'd strings
won't get free'd on server regeneration.

Comment 5 Adam Jackson 2005-12-09 06:45:41 UTC

Comment on attachment 2959 [details] [review]
Proposed patch

nominating for 7.  leaks, but not a lot, and prevents a crash.

Comment 6 Erik Andren 2006-04-22 23:59:11 UTC

Has this patch been merged into trunk?
(Keywording as patch)

Comment 7 Matthieu Herrb 2006-04-23 23:15:50 UTC

I just commited it to HEAD. Keeping the bug open if ajax wants to put it in 7.1.

Comment 8 Matthieu Herrb 2006-05-25 19:49:23 UTC

Can be closed 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.