Re: [PATCH] shell32: do not crash "wine control.exe nonexisting"
Marcus Meissner 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 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 julli...@winehq.org
Re: [PATCH] shell32: do not crash "wine control.exe nonexisting"
On Wed, Jun 06, 2012 at 11:56:06AM +0200, Jacek Caban wrote: > On 06/06/12 11:52, Alexandre Julliard wrote: > > Jacek Caban 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
Re: [PATCH] shell32: do not crash "wine control.exe nonexisting"
On 06/06/12 11:52, Alexandre Julliard wrote: > Jacek Caban 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
Re: [PATCH] shell32: do not crash "wine control.exe nonexisting"
Jacek Caban 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 julli...@winehq.org
Re: [PATCH] shell32: do not crash "wine control.exe nonexisting"
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
Re: [PATCH] shell32: do not crash "wine control.exe nonexisting"
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
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
