[Chromecast] Make SIGTERM/SIGINT handler safer

chromecast/browser/cast_browser_main_parts.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chromecast/browser/cast_browser_main_parts.h"
 
 #include <stddef.h>
 
 #include <string>
 
@@ skipping 58 matching lines @@
 #if defined(USE_AURA)
 #include "chromecast/graphics/cast_screen.h"
 #include "ui/aura/env.h"
 #include "ui/gfx/screen.h"
 #endif
 
 namespace {
 
 #if !defined(OS_ANDROID)
 int kSignalsToRunClosure[] = { SIGTERM, SIGINT, };
-
 // Closure to run on SIGTERM and SIGINT.
 base::Closure* g_signal_closure = NULL;
+base::PlatformThreadId g_main_thread_id;
+int g_quit_signal = 0;
 
 void RunClosureOnSignal(int signum) {
-  LOG(ERROR) << "Got signal " << signum;

[maclellant, 2015/12/28 19:30:32]

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");

[kmackay, 2015/12/29 18:23:01]

Done.

-  DCHECK(g_signal_closure);
-  // Expect main thread got this signal. Otherwise, weak_ptr of run_loop will
-  // crash the process.
+  if (base::PlatformThread::CurrentId() != g_main_thread_id)
+    return;
+  if (!g_signal_closure)
+    return;
+  g_quit_signal = signum;
   g_signal_closure->Run();
 }
 
-void RegisterClosureOnSignal(const base::Closure& closure) {
+void RegisterClosureOnSignal(base::Closure* closure) {
+  DCHECK(closure);
   DCHECK(!g_signal_closure);
   DCHECK_GT(arraysize(kSignalsToRunClosure), 0U);
 
-  // Allow memory leak by intention.
-  g_signal_closure = new base::Closure(closure);
+  g_signal_closure = closure;
+  g_main_thread_id = base::PlatformThread::CurrentId();
 
   struct sigaction sa_new;
   memset(&sa_new, 0, sizeof(sa_new));
   sa_new.sa_handler = RunClosureOnSignal;
   sigfillset(&sa_new.sa_mask);
   sa_new.sa_flags = SA_RESTART;
 
-  for (size_t i = 0; i < arraysize(kSignalsToRunClosure); i++) {
+  for (int sig : kSignalsToRunClosure) {
     struct sigaction sa_old;
-    if (sigaction(kSignalsToRunClosure[i], &sa_new, &sa_old) == -1) {
+    if (sigaction(sig, &sa_new, &sa_old) == -1) {
       NOTREACHED();
     } else {
       DCHECK_EQ(sa_old.sa_handler, SIG_DFL);

[maclellant, 2015/12/28 19:30:32]

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?

[kmackay, 2015/12/29 18:23:01]

I don't think so. Note that this check is not new code.

     }
   }
 
   // Get the first signal to exit when the parent process dies.
   prctl(PR_SET_PDEATHSIG, kSignalsToRunClosure[0]);
 }
 
+void DeregisterClosureOnSignal(base::Closure* closure) {
+  DCHECK_EQ(g_signal_closure, closure);
+  DCHECK_EQ(g_main_thread_id, base::PlatformThread::CurrentId());
+  g_signal_closure = nullptr;

[maclellant, 2015/12/28 19:30:32]

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.

[kmackay, 2015/12/29 18:23:01]

Right, I think the previous code with the "memory leak" is actually better.

+}
+
 const int kKillOnAlarmTimeoutSec = 5;  // 5 seconds
 
 void KillOnAlarm(int signum) {
   LOG(ERROR) << "Got alarm signal for termination: " << signum;
   raise(SIGKILL);
 }
 
 void RegisterKillOnAlarm(int timeout_seconds) {
   struct sigaction sa_new;
   memset(&sa_new, 0, sizeof(sa_new));
@@ skipping 258 matching lines @@
 }
 
 bool CastBrowserMainParts::MainMessageLoopRun(int* result_code) {
 #if defined(OS_ANDROID)
   // Android does not use native main MessageLoop.
   NOTREACHED();
   return true;
 #else
   base::RunLoop run_loop;
   base::Closure quit_closure(run_loop.QuitClosure());
-  RegisterClosureOnSignal(quit_closure);
+  RegisterClosureOnSignal(&quit_closure);
 
   // If parameters_.ui_task is not NULL, we are running browser tests.
   if (parameters_.ui_task) {
     base::MessageLoop* message_loop = base::MessageLoopForUI::current();
     message_loop->PostTask(FROM_HERE, *parameters_.ui_task);
     message_loop->PostTask(FROM_HERE, quit_closure);
   }
 
   run_loop.Run();
+  DeregisterClosureOnSignal(&quit_closure);
+  if (g_quit_signal)
+    LOG(INFO) << "Quitting due to signal " << g_quit_signal;
 
   // Once the main loop has stopped running, we give the browser process a few
   // seconds to stop cast service and finalize all resources. If a hang occurs
   // and cast services refuse to terminate successfully, then we SIGKILL the
   // current process to avoid indefinte hangs.
   RegisterKillOnAlarm(kKillOnAlarmTimeoutSec);
 
   cast_browser_process_->cast_service()->Stop();
   return true;
 #endif
@@ skipping 11 matching lines @@
 #if defined(USE_AURA)
   aura::Env::DeleteInstance();
 #endif
 
   DeregisterKillOnAlarm();
 #endif
 }
 
 }  // namespace shell
 }  // namespace chromecast