Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1542233002:
[Chromecast] Make SIGTERM/SIGINT handler safer (Closed)
|
Created:
5 years ago by kmackay Modified:
4 years, 11 months ago CC:
chromium-reviews, gunsch+watch_chromium.org, lcwu+watch_chromium.org, halliwell+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Chromecast] Make SIGTERM/SIGINT handler safer
* Use signal-safe logging.
* Ignore signals that are not to the main thread.
As far as I can tell, the runloop's quit closure is safe to run
in a signal handler, so that should be OK.
BUG= internal b/26253582
Committed: https://crrev.com/318de4b00ef94836116758d2bba1892fb0eb246c
Cr-Commit-Position: refs/heads/master@{#367089}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Use signal-safe logging #
Total comments: 1
Messages
Total messages: 14 (6 generated)
kmackay@chromium.org changed reviewers: + byungchul@chromium.org, halliwell@chromium.org, maclellant@chromium.org
On 2015/12/22 23:30:07, kmackay wrote: lgtm
https://codereview.chromium.org/1542233002/diff/1/chromecast/browser/cast_bro... File chromecast/browser/cast_browser_main_parts.cc (left): https://codereview.chromium.org/1542233002/diff/1/chromecast/browser/cast_bro... chromecast/browser/cast_browser_main_parts.cc:84: LOG(ERROR) << "Got signal " << signum; This printing is still useful to detect the cases where a signal comes in, but the main message loop locks up and never exits (this has happened before). I would print out simple raw log. RAW_LOG(INFO, "Received close signal"); https://codereview.chromium.org/1542233002/diff/1/chromecast/browser/cast_bro... File chromecast/browser/cast_browser_main_parts.cc (right): https://codereview.chromium.org/1542233002/diff/1/chromecast/browser/cast_bro... chromecast/browser/cast_browser_main_parts.cc:112: DCHECK_EQ(sa_old.sa_handler, SIG_DFL); Since old handler is never unregistered this will crash if CastBrowserMainParts stops the main loop and then tries to start it again. Does that ever happen? https://codereview.chromium.org/1542233002/diff/1/chromecast/browser/cast_bro... chromecast/browser/cast_browser_main_parts.cc:123: g_signal_closure = nullptr; Technically this is unsafe since the signal handler for SIGTERM and SIGINT are still registered, so while setting it to NULL a signal may come in and the new global value is not guaranteed to be valid inside the signal handler (only sig_atomic_t and C++11 atomic types allow that). But realistically I don't think this should be a problem since it's guarded on the main thread and Closure remains alive for some time.
https://codereview.chromium.org/1542233002/diff/1/chromecast/browser/cast_bro... File chromecast/browser/cast_browser_main_parts.cc (left): https://codereview.chromium.org/1542233002/diff/1/chromecast/browser/cast_bro... chromecast/browser/cast_browser_main_parts.cc:84: LOG(ERROR) << "Got signal " << signum; On 2015/12/28 19:30:32, maclellant wrote: > This printing is still useful to detect the cases where a signal comes in, but > the main message loop locks up and never exits (this has happened before). I > would print out simple raw log. > > RAW_LOG(INFO, "Received close signal"); Done. https://codereview.chromium.org/1542233002/diff/1/chromecast/browser/cast_bro... File chromecast/browser/cast_browser_main_parts.cc (right): https://codereview.chromium.org/1542233002/diff/1/chromecast/browser/cast_bro... chromecast/browser/cast_browser_main_parts.cc:112: DCHECK_EQ(sa_old.sa_handler, SIG_DFL); On 2015/12/28 19:30:32, maclellant wrote: > Since old handler is never unregistered this will crash if CastBrowserMainParts > stops the main loop and then tries to start it again. Does that ever happen? I don't think so. Note that this check is not new code. https://codereview.chromium.org/1542233002/diff/1/chromecast/browser/cast_bro... chromecast/browser/cast_browser_main_parts.cc:123: g_signal_closure = nullptr; On 2015/12/28 19:30:32, maclellant wrote: > Technically this is unsafe since the signal handler for SIGTERM and SIGINT are > still registered, so while setting it to NULL a signal may come in and the new > global value is not guaranteed to be valid inside the signal handler (only > sig_atomic_t and C++11 atomic types allow that). > > But realistically I don't think this should be a problem since it's guarded on > the main thread and Closure remains alive for some time. Right, I think the previous code with the "memory leak" is actually better.
Description was changed from ========== [Chromecast] Make SIGTERM/SIGINT handler safer * Don't log anything in the signal handler. * Ignore signals that are not to the main thread. As far as I can tell, the runloop's quit closure is safe to run in a signal handler, so that should be OK. BUG= internal b/26253582 ========== to ========== [Chromecast] Make SIGTERM/SIGINT handler safer * Use signal-safe logging. * Ignore signals that are not to the main thread. As far as I can tell, the runloop's quit closure is safe to run in a signal handler, so that should be OK. BUG= internal b/26253582 ==========
LGTM https://codereview.chromium.org/1542233002/diff/20001/chromecast/browser/cast... File chromecast/browser/cast_browser_main_parts.cc (right): https://codereview.chromium.org/1542233002/diff/20001/chromecast/browser/cast... chromecast/browser/cast_browser_main_parts.cc:91: strncat(message, sys_siglist[signum], sizeof(message) - strlen(message) - 1); I was about to reply that string library functions aren't async-safe, but it seems in almost all implementations they are, but haven't yet been added to the signal-safe list.
The CQ bit was checked by kmackay@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from halliwell@chromium.org Link to the patchset: https://codereview.chromium.org/1542233002/#ps20001 (title: "Use signal-safe logging")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1542233002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1542233002/20001
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Make SIGTERM/SIGINT handler safer * Use signal-safe logging. * Ignore signals that are not to the main thread. As far as I can tell, the runloop's quit closure is safe to run in a signal handler, so that should be OK. BUG= internal b/26253582 ========== to ========== [Chromecast] Make SIGTERM/SIGINT handler safer * Use signal-safe logging. * Ignore signals that are not to the main thread. As far as I can tell, the runloop's quit closure is safe to run in a signal handler, so that should be OK. BUG= internal b/26253582 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Make SIGTERM/SIGINT handler safer * Use signal-safe logging. * Ignore signals that are not to the main thread. As far as I can tell, the runloop's quit closure is safe to run in a signal handler, so that should be OK. BUG= internal b/26253582 ========== to ========== [Chromecast] Make SIGTERM/SIGINT handler safer * Use signal-safe logging. * Ignore signals that are not to the main thread. As far as I can tell, the runloop's quit closure is safe to run in a signal handler, so that should be OK. BUG= internal b/26253582 Committed: https://crrev.com/318de4b00ef94836116758d2bba1892fb0eb246c Cr-Commit-Position: refs/heads/master@{#367089} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/318de4b00ef94836116758d2bba1892fb0eb246c Cr-Commit-Position: refs/heads/master@{#367089} |
