PATCH Check module version each time it is loaded
Peter Hutterer
peter.hutterer at who-t.net
Thu Oct 13 16:35:14 PDT 2011
On Mon, Oct 10, 2011 at 10:10:23AM +0200, Michal Suchanek wrote:
> On 10 October 2011 05:38, Peter Hutterer <peter.hutterer at who-t.net> wrote:
> > On Fri, Oct 07, 2011 at 02:58:35PM +0200, Michal Suchanek wrote:
> >> Hello,
> >>
> >> The X server "duplicates" modules by loading them again from the disk.
> >>
> >> Since this "duplication" happens on every input device hotplug, possibly weeks
> >> after the X server has been started a different module might have been
> >> installed by that time (eg. due to upgrading distribution packages) the X
> >> server should check the module every time it is loaded to prevent weird issues
> >> due to modules compiled for different ABI.
> >>
> >> I compiled an X server with the patch and it worked but I have no idea how to
> >> test the error paths.
> >>
> >> https://bugs.freedesktop.org/attachment.cgi?id=51585
> >> https://bugs.freedesktop.org/show_bug.cgi?id=41182
> >
> > fwiw, last time I tried this (about a year or so ago), the problem was
> > dlopen(). even if you replace the file, dlopen() will simply re-load the
> > original file. Not sure if this has been fixed yet but that's what you need
> > to check first (would be simple to verify with a test program)
> >
>
> If it reloaded the original file no problem would occur.
>
> There was a reference counting layer in the loader which was removed
> supposedly because dlopen does just that.
>
> Anyway, here is much simpler way to fix it.
>
> Instead of the previous patch apply 2-5.
>
> Applied to 1.11 branch X server and tested that unplugging and
> replugging a wacom tablet works.
>
> That's the only driver I know of for which all devices can be removed.
so what's the behaviour now if you remove all devices? does it reload from
the disk or reload the original module?
> From 15e191dd91e09549ea2f96495f9f89ea0fd959ec Mon Sep 17 00:00:00 2001
> From: Michal Suchanek <hramrach at centrum.cz>
> Date: Sat, 8 Oct 2011 14:12:59 +0200
> Subject: [PATCH 2/5] Document DuplicateModule function.
>
> ---
> hw/xfree86/doc/ddxDesign.xml | 12 ++++++++++++
> 1 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/hw/xfree86/doc/ddxDesign.xml b/hw/xfree86/doc/ddxDesign.xml
> index a4baad5..6afdc56 100644
> --- a/hw/xfree86/doc/ddxDesign.xml
> +++ b/hw/xfree86/doc/ddxDesign.xml
> @@ -5495,6 +5495,18 @@ typedef struct {
>
> <blockquote><para>
> <programlisting>
> + pointer DuplicateModule(pointer mod);
this prototype doesn't match the one in the code.
I'd like to see the documentation in the code as well, that's the place
where people are more likely to encounter it.
> + </programlisting>
> + <blockquote><para>
> + This function creates a copy of the module description. All children are
> + also copied recursively. When you duplicate a module using this function
> + you are responsible for ensuring that no duplicated modules are used once
> + the master copy created with LoadModule or LoadSubModule is unloaded.
> + </para>
> + </blockquote></para></blockquote>
> +
> + <blockquote><para>
> + <programlisting>
> void UnloadModule(pointer mod);
> </programlisting>
> <blockquote><para>
> --
> 1.7.6.3
>
> From 4f26662dad12d5c1e20b941e8653b42be8985ef1 Mon Sep 17 00:00:00 2001
> From: Michal Suchanek <hramrach at centrum.cz>
> Date: Sat, 8 Oct 2011 14:13:33 +0200
> Subject: [PATCH 3/5] Unload submodules.
>
> ---
> hw/xfree86/common/xf86Helper.c | 6 ------
> 1 files changed, 0 insertions(+), 6 deletions(-)
>
> diff --git a/hw/xfree86/common/xf86Helper.c b/hw/xfree86/common/xf86Helper.c
> index a8aa316..4e9bcad 100644
> --- a/hw/xfree86/common/xf86Helper.c
> +++ b/hw/xfree86/common/xf86Helper.c
> @@ -1643,13 +1643,7 @@ xf86LoadOneModule(char *name, pointer opt)
> void
> xf86UnloadSubModule(pointer mod)
> {
> - /*
> - * This is disabled for now. The loader isn't smart enough yet to undo
> - * relocations.
> - */
> -#if 0
> UnloadSubModule(mod);
> -#endif
> }
>
> Bool
> --
> 1.7.6.3
Reviewed-by: Peter Hutterer <peter.hutterer at who-t.net>
> From b9937bbf6c984f245ca5c980a41e7f39dd5a8d82 Mon Sep 17 00:00:00 2001
> From: Michal Suchanek <hramrach at centrum.cz>
> Date: Sat, 8 Oct 2011 14:19:34 +0200
> Subject: [PATCH 4/5] Use UnloadModuleOrDriver for UnloadSubModule.
>
> ---
> hw/xfree86/loader/loadmod.c | 19 +++----------------
> 1 files changed, 3 insertions(+), 16 deletions(-)
>
> diff --git a/hw/xfree86/loader/loadmod.c b/hw/xfree86/loader/loadmod.c
> index 9f82099..119d2f4 100644
> --- a/hw/xfree86/loader/loadmod.c
> +++ b/hw/xfree86/loader/loadmod.c
> @@ -1092,23 +1092,8 @@ UnloadSubModule(pointer _mod)
> {
> ModuleDescPtr mod = (ModuleDescPtr)_mod;
>
> - if (mod == NULL || mod->name == NULL)
> - return;
> -
> - xf86MsgVerb(X_INFO, 3, "UnloadSubModule: \"%s\"\n", mod->name);
Can we keep this log message? UnloadModuleOrDriver could print based on
whether mod->parent is NULL.
> -
> - if ((mod->TearDownProc) && (mod->TearDownData))
> - mod->TearDownProc(mod->TearDownData);
> - LoaderUnload(mod->name, mod->handle);
> -
> RemoveChild(mod);
> -
> - if (mod->child)
> - UnloadModuleOrDriver(mod->child);
> -
> - free(mod->path);
> - free(mod->name);
> - free(mod);
> + UnloadModuleOrDriver(mod);
> }
>
> static void
> @@ -1135,6 +1120,8 @@ RemoveChild(ModuleDescPtr child)
> }
> if (mdp == child)
> prevsib->sib = child->sib;
> + child->parent = NULL;
> + child->sib = NULL;
> return;
> }
>
> --
> 1.7.6.3
Reviewed-by: Peter Hutterer <peter.hutterer at who-t.net> otherwise
> From d24dfb2dea14d1e869f52cf46204e8a869ea336a Mon Sep 17 00:00:00 2001
> From: Michal Suchanek <hramrach at centrum.cz>
> Date: Sat, 8 Oct 2011 14:26:24 +0200
> Subject: [PATCH 5/5] Do not uselessly reload modules in DuplicateModule
>
> The function does not initialize the module so it has no business
> loading it. If some user of DuplicateModule expects a module actually
> loaded they should use LoadModule.
> ---
> hw/xfree86/loader/loadmod.c | 20 ++++++++++----------
> 1 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/hw/xfree86/loader/loadmod.c b/hw/xfree86/loader/loadmod.c
> index 119d2f4..e6ba729 100644
> --- a/hw/xfree86/loader/loadmod.c
> +++ b/hw/xfree86/loader/loadmod.c
> @@ -94,6 +94,8 @@ const ModuleVersions LoaderVersionInfo = {
> ABI_FONT_VERSION
> };
>
> +static int ModuleDuplicated[] = {};
> +
> static void
> FreeStringList(char **paths)
> {
> @@ -785,7 +787,6 @@ ModuleDescPtr
> DuplicateModule(ModuleDescPtr mod, ModuleDescPtr parent)
> {
> ModuleDescPtr ret;
> - int errmaj, errmin;
>
> if (!mod)
> return NULL;
> @@ -794,14 +795,11 @@ DuplicateModule(ModuleDescPtr mod, ModuleDescPtr parent)
> if (ret == NULL)
> return NULL;
>
> - if (!(ret->handle = LoaderOpen(mod->path, &errmaj, &errmin))) {
> - free(ret);
> - return NULL;
> - }
> + ret->handle = mod->handle;
>
> ret->SetupProc = mod->SetupProc;
> ret->TearDownProc = mod->TearDownProc;
> - ret->TearDownData = NULL;
> + ret->TearDownData = ModuleDuplicated;
> ret->child = DuplicateModule(mod->child, ret);
> ret->sib = DuplicateModule(mod->sib, parent);
> ret->parent = parent;
> @@ -1066,7 +1064,7 @@ UnloadModule(pointer mod)
> static void
> UnloadModuleOrDriver(ModuleDescPtr mod)
> {
> - if (mod == (ModuleDescPtr) 1)
> + if (mod == (ModuleDescPtr) 1) /* WTF is this? */
> return;
>
> if (mod == NULL || mod->name == NULL)
> @@ -1074,9 +1072,11 @@ UnloadModuleOrDriver(ModuleDescPtr mod)
>
> xf86MsgVerb(X_INFO, 3, "UnloadModule: \"%s\"\n", mod->name);
>
> - if ((mod->TearDownProc) && (mod->TearDownData))
> - mod->TearDownProc(mod->TearDownData);
> - LoaderUnload(mod->name, mod->handle);
> + if (mod->TearDownData != ModuleDuplicated) {
> + if ((mod->TearDownProc) && (mod->TearDownData))
> + mod->TearDownProc(mod->TearDownData);
> + LoaderUnload(mod->name, mod->handle);
> + }
>
> if (mod->child)
> UnloadModuleOrDriver(mod->child);
> --
> 1.7.6.3
Reviewed-by: Peter Hutterer <peter.hutterer at who-t.net>
Cheers,
Peter
More information about the xorg-devel
mailing list