Crash the network or desktop process when an unknown IPC message is received.

remoting/host/daemon_process.cc

 // Copyright (c) 2012 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 "remoting/host/daemon_process.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/file_util.h"
@@ skipping 47 matching lines @@
   DCHECK(caller_task_runner()->BelongsToCurrentThread());
 
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(DaemonProcess, message)
     IPC_MESSAGE_HANDLER(ChromotingNetworkHostMsg_ConnectTerminal,
                         CreateDesktopSession)
     IPC_MESSAGE_HANDLER(ChromotingNetworkHostMsg_DisconnectTerminal,
                         CloseDesktopSession)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
+
+  if (!handled) {
+    LOG(ERROR) << "An unexpected IPC message received: type=" << message.type();
+    CrashNetworkProcess(FROM_HERE);
+  }
+
   return handled;

[Wez, 2013/03/04 23:17:09]

Do you know what IpcChannel itself does with the |handled| result?  e.g. does it
call OnChannelError(), and/or tear the channel down?

[alexeypa (please no reviews), 2013/03/05 00:01:44]

It ignores the returned value.

 }
 
 void DaemonProcess::OnPermanentError() {
   DCHECK(caller_task_runner()->BelongsToCurrentThread());
   Stop();
 }
 
 void DaemonProcess::CloseDesktopSession(int terminal_id) {
   DCHECK(caller_task_runner()->BelongsToCurrentThread());
 
   // Validate the supplied terminal ID. An attempt to close a desktop session
   // with an ID that couldn't possibly have been allocated is considered
   // a protocol error and the network process will be restarted.
   if (!IsTerminalIdKnown(terminal_id)) {
     LOG(ERROR) << "An invalid terminal ID. terminal_id=" << terminal_id;
     CrashNetworkProcess(FROM_HERE);
-    DeleteAllDesktopSessions();
     return;
   }
 
   DesktopSessionList::iterator i;
   for (i = desktop_sessions_.begin(); i != desktop_sessions_.end(); ++i) {
     if ((*i)->id() == terminal_id) {
       break;
     }
   }
 
@@ skipping 28 matching lines @@
 
 void DaemonProcess::CreateDesktopSession(int terminal_id) {
   DCHECK(caller_task_runner()->BelongsToCurrentThread());
 
   // Validate the supplied terminal ID. An attempt to create a desktop session
   // with an ID that could possibly have been allocated already is considered
   // a protocol error and the network process will be restarted.
   if (IsTerminalIdKnown(terminal_id)) {
     LOG(ERROR) << "An invalid terminal ID. terminal_id=" << terminal_id;
     CrashNetworkProcess(FROM_HERE);
-    DeleteAllDesktopSessions();
     return;
   }
 
   VLOG(1) << "Daemon: opened desktop session " << terminal_id;
   desktop_sessions_.push_back(
       DoCreateDesktopSession(terminal_id).release());
   next_terminal_id_ = std::max(next_terminal_id_, terminal_id + 1);
 }
 
 void DaemonProcess::CrashNetworkProcess(
     const tracked_objects::Location& location) {
   SendToNetwork(new ChromotingDaemonNetworkMsg_Crash(
       location.function_name(), location.file_name(), location.line_number()));

[Wez, 2013/03/04 23:17:09]

nit: You're not closing the IPC channel here, so DeleteAllDesktopSessions may
result in IPCs being dispatched to the network process, which you no longer
trust, which doesn't seem right.

[alexeypa (please no reviews), 2013/03/05 00:01:44]

Agree. The current implementation does not enforce anything. It is more of a
diagnostic tool.

+
+  DeleteAllDesktopSessions();

[Wez, 2013/02/28 23:53:14]

nit: We should really tear-down the IPC channel to the process and, after some
timeout, forcibly terminate the process.

[alexeypa (please no reviews), 2013/03/05 00:01:44]

Agree, but let's do it as a separate CL. I imagine it should be a part of the
worker process launcher.

[Wez, 2013/03/05 02:12:31]

SGTM.

 }
 
 void DaemonProcess::Initialize() {
   DCHECK(caller_task_runner()->BelongsToCurrentThread());
 
   // Get the name of the host configuration file.
   base::FilePath default_config_dir = remoting::GetConfigDir();
   base::FilePath config_path = default_config_dir.Append(kDefaultHostConfigFile);
   const CommandLine* command_line = CommandLine::ForCurrentProcess();
   if (command_line->HasSwitch(kHostConfigSwitchName)) {
@@ skipping 20 matching lines @@
 }
 
 void DaemonProcess::DeleteAllDesktopSessions() {
   while (!desktop_sessions_.empty()) {
     delete desktop_sessions_.front();
     desktop_sessions_.pop_front();
   }
 }
 
 }  // namespace remoting