From c83cc7f8628eeac44dcadb79ca3b80652770c11d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Ekl=C3=B6f?= Date: Tue, 12 Jan 2021 09:20:02 +0100 Subject: [PATCH 1/4] reaper: add reaper_del() - pid is no longer watched/monitored after this --- reaper.c | 11 +++++++++++ reaper.h | 1 + 2 files changed, 12 insertions(+) diff --git a/reaper.c b/reaper.c index 8ebd31a4..15e08539 100644 --- a/reaper.c +++ b/reaper.c @@ -98,6 +98,17 @@ reaper_add(struct reaper *reaper, pid_t pid, reaper_cb cb, void *cb_data) ((struct child){.pid = pid, .cb = cb, .cb_data = cb_data})); } +void +reaper_del(struct reaper *reaper, pid_t pid) +{ + tll_foreach(reaper->children, it) { + if (it->item.pid == pid) { + tll_remove(reaper->children, it); + break; + } + } +} + static bool fdm_reap(struct fdm *fdm, int fd, int events, void *data) { diff --git a/reaper.h b/reaper.h index f955aa44..2cd6dd6e 100644 --- a/reaper.h +++ b/reaper.h @@ -13,3 +13,4 @@ void reaper_destroy(struct reaper *reaper); typedef bool (*reaper_cb)(struct reaper *reaper, pid_t pid, void *data); void reaper_add(struct reaper *reaper, pid_t pid, reaper_cb cb, void *cb_data); +void reaper_del(struct reaper *reaper, pid_t pid); From fe12c54d353cb5d0d22ff800f025b96cdb7b665f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Ekl=C3=B6f?= Date: Tue, 12 Jan 2021 09:20:54 +0100 Subject: [PATCH 2/4] pgo: stub implementation of reaper_del() --- pgo/pgo.c | 1 + 1 file changed, 1 insertion(+) diff --git a/pgo/pgo.c b/pgo/pgo.c index 120b8628..39c20de0 100644 --- a/pgo/pgo.c +++ b/pgo/pgo.c @@ -136,6 +136,7 @@ notify_notify(const struct terminal *term, const char *title, const char *body) } void reaper_add(struct reaper *reaper, pid_t pid, reaper_cb cb, void *cb_data) {} +void reaper_del(struct reaper *reaper, pid_t pid) {} int From 6b539ba37305141e2139a13494e26652a69e3b60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Ekl=C3=B6f?= Date: Tue, 12 Jan 2021 09:21:01 +0100 Subject: [PATCH 3/4] =?UTF-8?q?term:=20remove=20client=20application?= =?UTF-8?q?=E2=80=99s=20pid=20from=20reaper=20when=20shutting=20down?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When we are shutting down the terminal, we explicitly wait for the child application to terminate (with a timeout, after which the child process is killed). I.e. there’s no need to let the reaper handle it. In fact, doing so leads to a crash since we will have destroyed (and thus free:d) the terminal instance when the reaper callback is called. --- terminal.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/terminal.c b/terminal.c index 0ea04fc5..c05325ee 100644 --- a/terminal.c +++ b/terminal.c @@ -1334,6 +1334,9 @@ term_shutdown(struct terminal *term) fdm_del(term->fdm, term->blink.fd); fdm_del(term->fdm, term->flash.fd); + /* We’ll deal with this explicitly */ + reaper_del(term->reaper, term->slave); + if (term->window != NULL && term->window->is_configured) fdm_del(term->fdm, term->ptmx); else From 214371458eac93a18d000bc60d3a2ffb08520a4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Ekl=C3=B6f?= Date: Tue, 12 Jan 2021 09:30:27 +0100 Subject: [PATCH 4/4] reaper: remove child from list *before* calling the user provided callback The user provided callback may call reaper_del(), in which case we will crash when we also try to remove the child from the list. Remove it from the list before the callback means reaper_del() (if called by the callback) will just loop through the entire list without finding the pid and thus do nothing. --- reaper.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/reaper.c b/reaper.c index 15e08539..9d98854e 100644 --- a/reaper.c +++ b/reaper.c @@ -138,23 +138,27 @@ fdm_reap(struct fdm *fdm, int fd, int events, void *data) } tll_foreach(reaper->children, it) { - struct child *child = &it->item; - pid_t pid = child->pid; + struct child *_child = &it->item; - if (pid != (pid_t)info.ssi_pid) + if (_child->pid != (pid_t)info.ssi_pid) continue; + /* Make sure we remove it *before* the callback, since it too + * may remove it */ + struct child child = it->item; + tll_remove(reaper->children, it); + bool reap_ourselves = true; - if (child->cb != NULL) - reap_ourselves = !child->cb(reaper, pid, child->cb_data); + if (child.cb != NULL) + reap_ourselves = !child.cb(reaper, child.pid, child.cb_data); if (reap_ourselves) { int result; - int res = waitpid(pid, &result, WNOHANG); + int res = waitpid(child.pid, &result, WNOHANG); if (res <= 0) { if (res < 0) - LOG_ERRNO("waitpid failed for pid=%d", pid); + LOG_ERRNO("waitpid failed for pid=%d", child.pid); continue; } @@ -165,8 +169,6 @@ fdm_reap(struct fdm *fdm, int fd, int events, void *data) else LOG_DBG("pid=%d: died of unknown resason", pid); } - - tll_remove(reaper->children, it); } if (hup)