Re: [PATCH] shell32: do not crash "wine control.exe nonexisting"
Hi Marcus, On 06/06/12 10:17, Marcus Meissner wrote:
Hi,
wine control.exe joy crashed in this line, as it should be wine control.exe joy.cpl.
The list is just not setup yet as we fail...
Ciao, Marcus --- dlls/shell32/control.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/dlls/shell32/control.c b/dlls/shell32/control.c index cb080d5..b3eddb6 100644 --- a/dlls/shell32/control.c +++ b/dlls/shell32/control.c @@ -55,7 +55,7 @@ void Control_UnloadApplet(CPlApplet* applet) } if (applet->proc) applet->proc(applet->hWnd, CPL_EXIT, 0L, 0L); FreeLibrary(applet->hModule); - list_remove( &applet->entry ); + if (applet->entry.next) list_remove( &applet->entry ); HeapFree(GetProcessHeap(), 0, applet->cmd); HeapFree(GetProcessHeap(), 0, applet); }
The way Wine standard list implementation works, it should not be NULL here. It seems that list_init call is missing where the CPlApplet is allocated. Cheers, Jacek
On Wed, Jun 06, 2012 at 11:31:52AM +0200, Jacek Caban wrote:
Hi Marcus,
On 06/06/12 10:17, Marcus Meissner wrote:
Hi,
wine control.exe joy crashed in this line, as it should be wine control.exe joy.cpl.
The list is just not setup yet as we fail...
Ciao, Marcus --- dlls/shell32/control.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/dlls/shell32/control.c b/dlls/shell32/control.c index cb080d5..b3eddb6 100644 --- a/dlls/shell32/control.c +++ b/dlls/shell32/control.c @@ -55,7 +55,7 @@ void Control_UnloadApplet(CPlApplet* applet) } if (applet->proc) applet->proc(applet->hWnd, CPL_EXIT, 0L, 0L); FreeLibrary(applet->hModule); - list_remove( &applet->entry ); + if (applet->entry.next) list_remove( &applet->entry ); HeapFree(GetProcessHeap(), 0, applet->cmd); HeapFree(GetProcessHeap(), 0, applet); }
The way Wine standard list implementation works, it should not be NULL here. It seems that list_init call is missing where the CPlApplet is allocated.
The error handling that calls this leaves the Control_LoadApplet function before it is attached to the list head. list_add_head( &panel->applets, &applet->entry ); is not called before leaving the function. No other list_ functions are used by Control_LoadApplet. Not sure if this is right, or if there should be a entry init? Ciao, Marcus
On 06/06/12 11:34, Marcus Meissner wrote:
On Wed, Jun 06, 2012 at 11:31:52AM +0200, Jacek Caban wrote:
Hi Marcus,
On 06/06/12 10:17, Marcus Meissner wrote:
Hi,
wine control.exe joy crashed in this line, as it should be wine control.exe joy.cpl.
The list is just not setup yet as we fail...
Ciao, Marcus --- dlls/shell32/control.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/dlls/shell32/control.c b/dlls/shell32/control.c index cb080d5..b3eddb6 100644 --- a/dlls/shell32/control.c +++ b/dlls/shell32/control.c @@ -55,7 +55,7 @@ void Control_UnloadApplet(CPlApplet* applet) } if (applet->proc) applet->proc(applet->hWnd, CPL_EXIT, 0L, 0L); FreeLibrary(applet->hModule); - list_remove( &applet->entry ); + if (applet->entry.next) list_remove( &applet->entry ); HeapFree(GetProcessHeap(), 0, applet->cmd); HeapFree(GetProcessHeap(), 0, applet); } The way Wine standard list implementation works, it should not be NULL here. It seems that list_init call is missing where the CPlApplet is allocated. The error handling that calls this leaves the Control_LoadApplet function before it is attached to the list head.
list_add_head( &panel->applets, &applet->entry );
is not called before leaving the function. No other list_ functions are used by Control_LoadApplet.
Not sure if this is right, or if there should be a entry init?
This usage of list is broken here as well. list_init should be called before list_add_head (so calling it early in initialization code will fix both problems). Cheers, Jacek
Jacek Caban <jacek(a)codeweavers.com> writes:
This usage of list is broken here as well. list_init should be called before list_add_head (so calling it early in initialization code will fix both problems).
There's no reason to call list_init on list entries, it should only be called on the actual list. -- Alexandre Julliard julliard(a)winehq.org
On 06/06/12 11:52, Alexandre Julliard wrote:
Jacek Caban <jacek(a)codeweavers.com> writes:
This usage of list is broken here as well. list_init should be called before list_add_head (so calling it early in initialization code will fix both problems). There's no reason to call list_init on list entries, it should only be called on the actual list.
Oh, right, sorry for misleading. I made wrong assumptions looking at the patch. Jacek
On Wed, Jun 06, 2012 at 11:56:06AM +0200, Jacek Caban wrote:
On 06/06/12 11:52, Alexandre Julliard wrote:
Jacek Caban <jacek(a)codeweavers.com> writes:
This usage of list is broken here as well. list_init should be called before list_add_head (so calling it early in initialization code will fix both problems). There's no reason to call list_init on list entries, it should only be called on the actual list.
Oh, right, sorry for misleading. I made wrong assumptions looking at the patch.
So, Alexandre, is my patch right or wrong? And if wrong, can you quickly fix it:) Ciao, Marcus
Marcus Meissner <meissner(a)suse.de> writes:
On Wed, Jun 06, 2012 at 11:56:06AM +0200, Jacek Caban wrote:
On 06/06/12 11:52, Alexandre Julliard wrote:
Jacek Caban <jacek(a)codeweavers.com> writes:
This usage of list is broken here as well. list_init should be called before list_add_head (so calling it early in initialization code will fix both problems). There's no reason to call list_init on list entries, it should only be called on the actual list.
Oh, right, sorry for misleading. I made wrong assumptions looking at the patch.
So, Alexandre, is my patch right or wrong? And if wrong, can you quickly fix it:)
The patch would work, but it's not very clean. It's better to avoid destroying objects that are not yet on the list. I have some more cleanups for that code that should address the issue. -- Alexandre Julliard julliard(a)winehq.org
participants (3)
-
Alexandre Julliard -
Jacek Caban -
Marcus Meissner