That's the intent, but the patch as is introduces a new bug - now we can have a double free somewhere else. :(
Removing the notifier from the list in the sigchld handler may be a better solution (a quick test appears ok), but I think there's still a tiny window where sigchld can fire just before the notifier is set up. I think we probably need to stop calling launch_desktop_shell_process() from inside a signal handler... The short description of the problem is: weston_client_launch() returns successfully (forked ok) we add a destroy listener for the new client sigchld fires when the forked process can't exec the sigchld handler starts over at weston_client_launch() we add the same destroy listener struct (still on a list!) to a new list Now two linked lists share pointers to that listener... at some point later the destroy notification fires and walks a broken linked list resulting in a segfault. On 22/08/14 05:47 PM, Eoff, Ullysses A wrote: > Does this fix https://bugs.freedesktop.org/show_bug.cgi?id=82957 ? > > ---- > U. Artie > >> -----Original Message----- >> From: wayland-devel [mailto:[email protected]] On >> Behalf Of Derek Foreman >> Sent: Friday, August 22, 2014 2:20 PM >> To: [email protected] >> Cc: Derek Foreman >> Subject: [PATCH] desktop-shell: No longer segfault on cleanup >> >> When desktop-shell's sigchld handler attempts to re-launch >> weston-desktop-shell it also registers a destroy notifier. >> >> This destroy notifier may still be on another linked list - their does not >> appear to be any guarantee that the destroy hander will have been called >> before the cleanup callback. >> >> So, when the cleanup callback is run before the destroy notifications are >> emitted, the list will become corrupted, causing a segfault when the >> notifications are emit. >> >> Since the destroy notifier just sets a pointer to NULL - which will happen >> in the cleanup handler anyway - I've simply removed the destroy handler. >> >> Signed-off-by: Derek Foreman <[email protected]> >> --- >> desktop-shell/shell.c | 16 ---------------- >> desktop-shell/shell.h | 1 - >> 2 files changed, 17 deletions(-) >> >> diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c >> index 99f3343..07700cf 100644 >> --- a/desktop-shell/shell.c >> +++ b/desktop-shell/shell.c >> @@ -5322,17 +5322,6 @@ desktop_shell_sigchld(struct weston_process *process, >> int status) >> } >> >> static void >> -desktop_shell_client_destroy(struct wl_listener *listener, void *data) >> -{ >> - struct desktop_shell *shell; >> - >> - shell = container_of(listener, struct desktop_shell, >> - child.client_destroy_listener); >> - >> - shell->child.client = NULL; >> -} >> - >> -static void >> launch_desktop_shell_process(void *data) >> { >> struct desktop_shell *shell = data; >> @@ -5344,11 +5333,6 @@ launch_desktop_shell_process(void *data) >> >> if (!shell->child.client) >> weston_log("not able to start %s\n", shell->client); >> - >> - shell->child.client_destroy_listener.notify = >> - desktop_shell_client_destroy; >> - wl_client_add_destroy_listener(shell->child.client, >> - &shell->child.client_destroy_listener); >> } >> >> static void >> diff --git a/desktop-shell/shell.h b/desktop-shell/shell.h >> index c6ea328..3ddaa89 100644 >> --- a/desktop-shell/shell.h >> +++ b/desktop-shell/shell.h >> @@ -137,7 +137,6 @@ struct desktop_shell { >> struct weston_process process; >> struct wl_client *client; >> struct wl_resource *desktop_shell; >> - struct wl_listener client_destroy_listener; >> >> unsigned deathcount; >> uint32_t deathstamp; >> -- >> 2.1.0.rc1 >> >> _______________________________________________ >> wayland-devel mailing list >> [email protected] >> http://lists.freedesktop.org/mailman/listinfo/wayland-devel > _______________________________________________ > wayland-devel mailing list > [email protected] > http://lists.freedesktop.org/mailman/listinfo/wayland-devel > _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
