https://bugs.freedesktop.org/attachment.cgi?id=51585
The patch is incompatible with the dummy video driver version 0.3.4.
There is a duble-free which happens in case of an error but when fixed
it still does not work:
[1238177.911] (II) LoadModule: "dummy"
[1238177.912] (II) Loading /usr/lib/xorg/modules/drivers/dummy_drv.so
[1238177.914] (II) Module dummy: vendor="X.Org Foundation"
[1238177.914] compiled for 1.11.0, module version = 0.3.4
[1238177.914] Module class: X.Org Video Driver
[1238177.914] ABI class: X.Org Video Driver, version 11.0
[1238177.919] (II) LoadModule: "void"
[1238177.924] (WW) Warning, couldn't open module void
[1238177.925] (II) UnloadModule: "void"
[1238177.925] (II) Unloading void
[1238177.927] (EE) Failed to load module "void" (module does not exist, 0)
[1238177.928] (II) DUMMY: Driver for Dummy chipsets: dummy
[1238177.932] (WW) Falling back to old probe method for dummy
[1238177.938] (II) Loading /usr/lib/xorg/modules/drivers/dummy_drv.so
[1238177.939] (II) Module dummy: vendor="X.Org Foundation"
[1238177.939] compiled for 1.11.0, module version = 0.3.4
[1238177.939] Module class: X.Org Video Driver
[1238177.939] ABI class: X.Org Video Driver, version 11.0
[1238177.940] (EE) LoadModule: Module dummy SetupProc failed
Obviously, the dummy was duplicated here.
SetupProc returns NULL which causes LoaderOpen failure and returns a NULL copy.
The patched-out part of DupliceteModule would just set the
TeardownData (which is what SetupProc is supposed to return) to NULL:
- if (!(ret->handle = LoaderOpen(mod->path, &errmaj, &errmin))) {
- free(ret);
+ if (!doLoaderOpen(ret, mod->path, NULL, NULL, &errmaj, &errmin)) {
+ UnloadModule(ret);
return NULL;
}
-
- ret->SetupProc = mod->SetupProc;
- ret->TearDownProc = mod->TearDownProc;
- ret->TearDownData = NULL;
ret->child = DuplicateModule(mod->child, ret);
ret->sib = DuplicateModule(mod->sib, parent);
ret->parent = parent;
Is there any guarantee that the the first copy of the module which
does have the teardown data is unloaded last, though?
[1238177.940] (II) UnloadModule: "dummy"
[1238177.940] (II) Unloading dummy
[1238177.945] (WW) VGA arbiter: cannot open kernel arbiter, no
multi-card support
[1238177.953] (II) DUMMY(0): Chipset is a DUMMY
[1238177.958] (**) DUMMY(0): Depth 24, (--) framebuffer bpp 32
..
[1238178.117] (II) DUMMY(0): Modeline "320x175"x85.3 15.75 320 336
368 416 175 191 192 222 doublescan +hsync -vsync (37.9 kHz d)
[1238178.119] (==) DUMMY(0): DPI set to (96, 96)
[1238178.120] (EE) DUMMY: Failed to load module "fb" (invalid
argument(s) to LoadModule(), 0)
[1238178.125] (EE) Screen(s) found, but none have a usable configuration.
dummy tries to load submodule fb on its failed copy which is NULL.
This would lead to segfault because neither dummy nor LoadSubModule
checked that the pointer was non-NULL.
Any ideas how to resolve this?
Thanks
Michal
From 457aae13aad2f0393ba8db6bc8af9d3d698271ba Mon Sep 17 00:00:00 2001
From: Michal Suchanek <[email protected]>
Date: Tue, 15 Mar 2011 11:04:13 +0100
Subject: [PATCH] Do full module probe on every LoaderOpen
There is no guarantee that a loaded module stays the same in the
filesystem since it was loaded for the first time. Opening the module
again without checking module version leads to all the interesting
issues the version check was supposed to avoid when the module changes
in the filesystem (eg. due to package upgrade).
---
hw/xfree86/loader/loadmod.c | 75 +++++++++++++++++++++++++++---------------
1 files changed, 48 insertions(+), 27 deletions(-)
Index: xserver/hw/xfree86/loader/loadmod.c
===================================================================
--- xserver.orig/hw/xfree86/loader/loadmod.c 2011-10-07 20:07:34.000000000 +0200
+++ xserver/hw/xfree86/loader/loadmod.c 2011-10-07 20:56:46.000000000 +0200
@@ -84,6 +84,9 @@
static ModuleDescPtr doLoadModule(const char *, const char *, const char **,
const char **, pointer,
const XF86ModReqInfo *, int *, int *);
+static int /*bool*/ doLoaderOpen(ModuleDescPtr ret, const char * module,
+ const XF86ModReqInfo * modreq, pointer options,
+ int * errmaj, int * errmin);
const ModuleVersions LoaderVersionInfo = {
XORG_VERSION_CURRENT,
@@ -747,6 +750,11 @@
{
ModuleDescPtr submod;
ModuleDescPtr parent = (ModuleDescPtr)_parent;
+ if (! parent) {
+ if (errmaj)
+ *errmaj = LDR_BADUSAGE;
+ return NULL;
+ }
xf86MsgVerb(X_INFO, 3, "Loading sub module \"%s\"\n", module);
@@ -787,6 +795,8 @@
ModuleDescPtr ret;
int errmaj, errmin;
+ /* FIXME may need options and modreq to reopen module correctly */
+
if (!mod)
return NULL;
@@ -794,20 +804,13 @@
if (ret == NULL)
return NULL;
- if (!(ret->handle = LoaderOpen(mod->path, &errmaj, &errmin))) {
- free(ret);
+ if (!doLoaderOpen(ret, mod->path, NULL, NULL, &errmaj, &errmin)) {
+ UnloadModule(ret);
return NULL;
}
-
- ret->SetupProc = mod->SetupProc;
- ret->TearDownProc = mod->TearDownProc;
- ret->TearDownData = NULL;
ret->child = DuplicateModule(mod->child, ret);
ret->sib = DuplicateModule(mod->sib, parent);
ret->parent = parent;
- ret->VersionInfo = mod->VersionInfo;
- ret->path = strdup(mod->path);
-
return ret;
}
@@ -824,7 +827,6 @@
const XF86ModReqInfo * modreq,
int *errmaj, int *errmin)
{
- XF86ModuleData *initdata = NULL;
char **pathlist = NULL;
char *found = NULL;
char *name = NULL;
@@ -867,7 +869,13 @@
*errmin = 0;
goto LoadModule_fail;
}
+
+ /* drop any explicit suffix from the module name */
+ p = strchr(name, '.');
+ if (p)
+ *p = '\0';
ret = NewModuleDesc(name);
+
if (!ret) {
if (errmaj)
*errmaj = LDR_NOMEM;
@@ -917,21 +925,45 @@
*errmin = 0;
goto LoadModule_fail;
}
- ret->handle = LoaderOpen(found, errmaj, errmin);
- if (ret->handle < 0)
+ if(! doLoaderOpen(ret, found, modreq, options, errmaj, errmin))
goto LoadModule_fail;
- ret->path = strdup(found);
+ else
+ goto LoadModule_exit;
- /* drop any explicit suffix from the module name */
- p = strchr(name, '.');
- if (p)
- *p = '\0';
+
+ LoadModule_fail:
+ UnloadModule(ret);
+ ret = NULL;
+
+ LoadModule_exit:
+ FreePathList(pathlist);
+ FreePatterns(patterns);
+ free(found);
+ free(name);
+
+ return ret;
+}
+
+static int /*bool*/ doLoaderOpen(ModuleDescPtr ret, const char * module,
+ const XF86ModReqInfo * modreq, pointer options,
+ int * errmaj, int * errmin)
+{
+ XF86ModuleData *initdata = NULL;
+ char *p = NULL;
+ int retval = 1;
+ if (!ret || !ret->name)
+ goto LoadModule_fail;
+
+ ret->handle = LoaderOpen(module, errmaj, errmin);
+ if (ret->handle < 0)
+ goto LoadModule_fail;
+ ret->path = strdup(module);
/*
* now check if the special data object <modulename>ModuleData is
* present.
*/
- if (asprintf(&p, "%sModuleData", name) == -1) {
+ if (asprintf(&p, "%sModuleData", ret->name) == -1) {
p = NULL;
if (errmaj)
*errmaj = LDR_NOMEM;
@@ -960,7 +992,7 @@
} else {
xf86Msg(X_ERROR,
"LoadModule: Module %s does not supply"
- " version information\n", module);
+ " version information\n", ret->name);
if (errmaj)
*errmaj = LDR_INVALID;
if (errmin)
@@ -979,7 +1011,7 @@
/* no initdata, fail the load */
xf86Msg(X_ERROR, "LoadModule: Module %s does not have a %s "
- "data object.\n", module, p);
+ "data object.\n", ret->name, p);
if (errmaj)
*errmaj = LDR_INVALID;
if (errmin)
@@ -989,26 +1021,20 @@
if (ret->SetupProc) {
ret->TearDownData = ret->SetupProc(ret, options, errmaj, errmin);
if (!ret->TearDownData) {
+ xf86Msg(X_ERROR, "LoadModule: Module %s SetupProc failed\n", ret->name, p);
goto LoadModule_fail;
}
} else if (options) {
xf86Msg(X_WARNING, "Module Options present, but no SetupProc "
- "available for %s\n", module);
+ "available for %s\n", ret->name);
}
goto LoadModule_exit;
LoadModule_fail:
- UnloadModule(ret);
- ret = NULL;
-
+ retval = 0;
LoadModule_exit:
- FreePathList(pathlist);
- FreePatterns(patterns);
- free(found);
- free(name);
- free(p);
-
- return ret;
+ free(p);
+ return retval;
}
/*
_______________________________________________
[email protected]: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
