Make sanity-checking consistent for events received via IPC and on-the-wire.

- Make DesktopSessionAgent checks consistent with HostEvent/ControlDispatcher.
- Move additional checks into platform-specific EventExecutors.

BUG=177559


Review URL: https://chromiumcodereview.appspot.com/12320051

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@184920 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 86 insertions, 100 deletions

diff --git a/remoting/host/clipboard_mac.mm b/remoting/host/clipboard_mac.mm
index b782421..8baafb6 100644
--- a/remoting/host/clipboard_mac.mm
+++ b/remoting/host/clipboard_mac.mm
@@ -12,6 +12,7 @@
 #include "base/sys_string_conversions.h"
 #include "base/timer.h"
 #include "remoting/base/constants.h"
+#include "remoting/base/util.h"
 #include "remoting/proto/event.pb.h"
 #include "remoting/protocol/clipboard_stub.h"
 
@@ -72,7 +73,10 @@ void ClipboardMac::Start(scoped_ptr<protocol::ClipboardStub> client_clipboard) {
 
 void ClipboardMac::InjectClipboardEvent(const protocol::ClipboardEvent& event) {
   // Currently we only handle UTF-8 text.
-  if (event.mime_type().compare(kMimeTypeTextUtf8)) {
+  if (event.mime_type().compare(kMimeTypeTextUtf8) != 0)
+    return;
+  if (!StringIsUtf8(event.data().c_str(), event.data().length())) {
+    LOG(ERROR) << "ClipboardEvent data is not UTF-8 encoded.";
     return;
   }
 
diff --git a/remoting/host/clipboard_win.cc b/remoting/host/clipboard_win.cc
index 7549f120..3b766c5 100644
--- a/remoting/host/clipboard_win.cc
+++ b/remoting/host/clipboard_win.cc
@@ -196,13 +196,17 @@ void ClipboardWin::Stop() {
 
 void ClipboardWin::InjectClipboardEvent(
     const protocol::ClipboardEvent& event) {
-  if (!hwnd_) {
+  if (!hwnd_)
     return;
-  }
+
   // Currently we only handle UTF-8 text.
-  if (event.mime_type().compare(kMimeTypeTextUtf8)) {
+  if (event.mime_type().compare(kMimeTypeTextUtf8) != 0)
+    return;
+  if (!StringIsUtf8(event.data().c_str(), event.data().length())) {
+    LOG(ERROR) << "ClipboardEvent: data is not UTF-8 encoded.";
     return;
   }
+
   string16 text = UTF8ToUTF16(ReplaceLfByCrLf(event.data()));
 
   ScopedClipboard clipboard;
diff --git a/remoting/host/desktop_session_agent.cc b/remoting/host/desktop_session_agent.cc
index 5fbba57..297bc87 100644
--- a/remoting/host/desktop_session_agent.cc
+++ b/remoting/host/desktop_session_agent.cc
@@ -12,7 +12,6 @@
 #include "media/video/capture/screen/screen_capture_data.h"
 #include "remoting/base/auto_thread_task_runner.h"
 #include "remoting/base/constants.h"
-#include "remoting/base/util.h"
 #include "remoting/host/audio_capturer.h"
 #include "remoting/host/chromoting_messages.h"
 #include "remoting/host/desktop_environment.h"
@@ -31,11 +30,6 @@ namespace remoting {
 
 namespace {
 
-// USB to XKB keycode map table.
-#define USB_KEYMAP(usb, xkb, win, mac) {usb, xkb}
-#include "ui/base/keycodes/usb_keycode_map.h"
-#undef USB_KEYMAP
-
 // Routes local clipboard events though the IPC channel to the network process.
 class DesktopSesssionClipboardStub : public protocol::ClipboardStub {
  public:
@@ -380,8 +374,7 @@ void DesktopSessionAgent::OnInvalidateRegion(
   SkRegion invalid_region;
   for (std::vector<SkIRect>::const_iterator i = invalid_rects.begin();
       i != invalid_rects.end(); ++i) {
-    // Validate each rectange and clip it to the frame bounds. If the rectangle
-    // is not valid it is ignored.
+    // Validate each rectange and clip it to the frame bounds.
     SkIRect rect;
     if (rect.intersect(*i, bounds)) {
       invalid_region.op(rect, SkRegion::kUnion_Op);
@@ -419,15 +412,8 @@ void DesktopSessionAgent::OnInjectClipboardEvent(
     return;
   }
 
-  // Currently we only handle UTF-8 text.
-  if (event.mime_type().compare(kMimeTypeTextUtf8) != 0)
-    return;
-
-  if (!StringIsUtf8(event.data().c_str(), event.data().length())) {
-    LOG(ERROR) << "ClipboardEvent: data is not UTF-8 encoded.";
-    return;
-  }
-
+  // InputStub implementations must verify events themselves, so we don't need
+  // verification here. This matches HostEventDispatcher.
   event_executor_->InjectClipboardEvent(event);
 }
 
@@ -441,12 +427,10 @@ void DesktopSessionAgent::OnInjectKeyEvent(
     return;
   }
 
-  // Ignore unknown keycodes.
-  if (event.has_usb_keycode() &&
-      (UsbKeycodeToNativeKeycode(event.usb_keycode()) ==
-           InvalidNativeKeycode())) {
-    LOG(ERROR) << "KeyEvent: unknown USB keycode: "
-               << std::hex << event.usb_keycode() << std::dec;
+  // InputStub implementations must verify events themselves, so we need only
+  // basic verification here. This matches HostEventDispatcher.
+  if (!event.has_usb_keycode() || !event.has_pressed()) {
+    LOG(ERROR) << "Received invalid key event.";
     return;
   }
 
@@ -463,20 +447,8 @@ void DesktopSessionAgent::OnInjectMouseEvent(
     return;
   }
 
-  // Validate the specified button index.
-  if (event.has_button() &&
-      !(protocol::MouseEvent::BUTTON_LEFT <= event.button() &&
-          event.button() < protocol::MouseEvent::BUTTON_MAX)) {
-    LOG(ERROR) << "MouseEvent: unknown button: " << event.button();
-    return;
-  }
-
-  // Do not allow negative coordinates.
-  if (event.has_x())
-    event.set_x(std::max(0, event.x()));
-  if (event.has_y())
-    event.set_y(std::max(0, event.y()));
-
+  // InputStub implementations must verify events themselves, so we don't need
+  // verification here. This matches HostEventDispatcher.
   remote_input_filter_->InjectMouseEvent(event);
 }
 
diff --git a/remoting/host/event_executor_linux.cc b/remoting/host/event_executor_linux.cc
index e668b16..84af5a9 100644
--- a/remoting/host/event_executor_linux.cc
+++ b/remoting/host/event_executor_linux.cc
@@ -196,13 +196,14 @@ void EventExecutorLinux::Core::InjectClipboardEvent(
     return;
   }
 
+  // |clipboard_| will ignore unknown MIME-types, and verify the data's format.
   clipboard_->InjectClipboardEvent(event);
 }
 
 void EventExecutorLinux::Core::InjectKeyEvent(const KeyEvent& event) {
   // HostEventDispatcher should filter events missing the pressed field.
-  DCHECK(event.has_pressed());
-  DCHECK(event.has_usb_keycode());
+  if (!event.has_pressed() || !event.has_usb_keycode())
+    return;
 
   if (!task_runner_->BelongsToCurrentThread()) {
     task_runner_->PostTask(FROM_HERE,
@@ -211,6 +212,7 @@ void EventExecutorLinux::Core::InjectKeyEvent(const KeyEvent& event) {
   }
 
   int keycode = UsbKeycodeToNativeKeycode(event.usb_keycode());
+
   VLOG(3) << "Converting USB keycode: " << std::hex << event.usb_keycode()
           << " to keycode: " << keycode << std::dec;
 
@@ -290,13 +292,16 @@ void EventExecutorLinux::Core::InjectMouseEvent(const MouseEvent& event) {
         inject_motion = false;
     }
 
-    latest_mouse_position_ = new_mouse_position;
-
     if (inject_motion) {
-      VLOG(3) << "Moving mouse to " << event.x()
-              << "," << event.y();
+      latest_mouse_position_ =
+          SkIPoint::Make(std::max(0, new_mouse_position.x()),
+                         std::max(0, new_mouse_position.y()));
+
+      VLOG(3) << "Moving mouse to " << latest_mouse_position_.x()
+              << "," << latest_mouse_position_.y();
       XTestFakeMotionEvent(display_, DefaultScreen(display_),
-                           event.x(), event.y(),
+                           latest_mouse_position_.x(),
+                           latest_mouse_position_.y(),
                            CurrentTime);
     }
   }
@@ -358,9 +363,8 @@ void EventExecutorLinux::Core::InitMouseButtonMap() {
   }
   for (int i = 0; i < num_buttons; i++) {
     // Reverse the mapping.
-    if (pointer_mapping[i] > 0 && pointer_mapping[i] <= kNumPointerButtons) {
+    if (pointer_mapping[i] > 0 && pointer_mapping[i] <= kNumPointerButtons)
       pointer_button_map_[pointer_mapping[i] - 1] = i + 1;
-    }
   }
   for (int i = 0; i < kNumPointerButtons; i++) {
     if (pointer_button_map_[i] == -1)
diff --git a/remoting/host/event_executor_mac.cc b/remoting/host/event_executor_mac.cc
index bdd6034..8a5286b 100644
--- a/remoting/host/event_executor_mac.cc
+++ b/remoting/host/event_executor_mac.cc
@@ -4,6 +4,7 @@
 
 #include "remoting/host/event_executor.h"
 
+#include <algorithm>
 #include <ApplicationServices/ApplicationServices.h>
 #include <Carbon/Carbon.h>
 
@@ -147,13 +148,14 @@ void EventExecutorMac::Core::InjectClipboardEvent(const ClipboardEvent& event) {
     return;
   }
 
+  // |clipboard_| will ignore unknown MIME-types, and verify the data's format.
   clipboard_->InjectClipboardEvent(event);
 }
 
 void EventExecutorMac::Core::InjectKeyEvent(const KeyEvent& event) {
   // HostEventDispatcher should filter events missing the pressed field.
-  DCHECK(event.has_pressed());
-  DCHECK(event.has_usb_keycode());
+  if (!event.has_pressed() || !event.has_usb_keycode())
+    return;
 
   int keycode = UsbKeycodeToNativeKeycode(event.usb_keycode());
 
@@ -170,9 +172,8 @@ void EventExecutorMac::Core::InjectKeyEvent(const KeyEvent& event) {
 #pragma clang diagnostic ignored "-Wdeprecated-declarations"
   CGError error = CGPostKeyboardEvent(0, keycode, event.pressed());
 #pragma clang diagnostic pop
-  if (error != kCGErrorSuccess) {
+  if (error != kCGErrorSuccess)
     LOG(WARNING) << "CGPostKeyboardEvent error " << error;
-  }
 }
 
 void EventExecutorMac::Core::InjectMouseEvent(const MouseEvent& event) {
@@ -181,8 +182,6 @@ void EventExecutorMac::Core::InjectMouseEvent(const MouseEvent& event) {
     // display, whereas our coordinate scheme places (0,0) at the top-left of
     // the bounding rectangle around all the displays, so we need to translate
     // accordingly.
-    // TODO(wez): Move display config tracking into a separate class used both
-    // here and in the Capturer.
 
     // Set the mouse position assuming single-monitor.
     mouse_pos_ = SkIPoint::Make(event.x(), event.y());
@@ -200,6 +199,13 @@ void EventExecutorMac::Core::InjectMouseEvent(const MouseEvent& event) {
     mouse_pos_ += SkIPoint::Make(desktop_config.pixel_bounds.left(),
                                  desktop_config.pixel_bounds.top());
 
+    // Constrain the mouse position to the desktop coordinates.
+    mouse_pos_ = SkIPoint::Make(
+       std::max(desktop_config.pixel_bounds.left(),
+           std::min(desktop_config.pixel_bounds.right(), mouse_pos_.x())),
+       std::max(desktop_config.pixel_bounds.top(),
+           std::min(desktop_config.pixel_bounds.bottom(), mouse_pos_.y())));
+
     // Convert from pixel to Density Independent Pixel coordinates.
     mouse_pos_ = SkIPoint::Make(
         SkScalarRound(mouse_pos_.x() / desktop_config.dip_to_pixel_scale),
@@ -239,9 +245,8 @@ void EventExecutorMac::Core::InjectMouseEvent(const MouseEvent& event) {
                                    (mouse_button_state_ & RightBit) != 0,
                                    (mouse_button_state_ & MiddleBit) != 0);
 #pragma clang diagnostic pop
-  if (error != kCGErrorSuccess) {
+  if (error != kCGErrorSuccess)
     LOG(WARNING) << "CGPostMouseEvent error " << error;
-  }
 
   if (event.has_wheel_delta_x() && event.has_wheel_delta_y()) {
     int delta_x = static_cast<int>(event.wheel_delta_x());
@@ -249,9 +254,8 @@ void EventExecutorMac::Core::InjectMouseEvent(const MouseEvent& event) {
     base::mac::ScopedCFTypeRef<CGEventRef> event(
         CGEventCreateScrollWheelEvent(
             NULL, kCGScrollEventUnitPixel, 2, delta_y, delta_x));
-    if (event) {
+    if (event)
       CGEventPost(kCGSessionEventTap, event);
-    }
   }
 }
 
diff --git a/remoting/host/event_executor_win.cc b/remoting/host/event_executor_win.cc
index 02e0f4c..6398a74db 100644
--- a/remoting/host/event_executor_win.cc
+++ b/remoting/host/event_executor_win.cc
@@ -11,6 +11,7 @@
 #include "base/location.h"
 #include "base/memory/ref_counted.h"
 #include "base/single_thread_task_runner.h"
+#include "remoting/base/util.h"
 #include "remoting/host/clipboard.h"
 #include "remoting/proto/event.pb.h"
 // SkSize.h assumes that stdint.h-style types are already defined.
@@ -128,6 +129,7 @@ void EventExecutorWin::Core::InjectClipboardEvent(const ClipboardEvent& event) {
     return;
   }
 
+  // |clipboard_| will ignore unknown MIME-types, and verify the data's format.
   clipboard_->InjectClipboardEvent(event);
 }
 
@@ -177,12 +179,21 @@ EventExecutorWin::Core::~Core() {
 
 void EventExecutorWin::Core::HandleKey(const KeyEvent& event) {
   // HostEventDispatcher should filter events missing the pressed field.
-  DCHECK(event.has_pressed());
-  DCHECK(event.has_usb_keycode());
+  if (!event.has_pressed() || !event.has_usb_keycode())
+    return;
 
   // Reset the system idle suspend timeout.
   SetThreadExecutionState(ES_SYSTEM_REQUIRED);
 
+  int scancode = UsbKeycodeToNativeKeycode(event.usb_keycode());
+
+  VLOG(3) << "Converting USB keycode: " << std::hex << event.usb_keycode()
+          << " to scancode: " << scancode << std::dec;
+
+  // Ignore events which can't be mapped.
+  if (scancode == InvalidNativeKeycode())
+    return;
+
   // Populate the a Windows INPUT structure for the event.
   INPUT input;
   memset(&input, 0, sizeof(input));
@@ -192,26 +203,16 @@ void EventExecutorWin::Core::HandleKey(const KeyEvent& event) {
   if (!event.pressed())
     input.ki.dwFlags |= KEYEVENTF_KEYUP;
 
-  int scancode = UsbKeycodeToNativeKeycode(event.usb_keycode());
-  VLOG(3) << "Converting USB keycode: " << std::hex << event.usb_keycode()
-          << " to scancode: " << scancode << std::dec;
-
-  // Ignore events which can't be mapped.
-  if (scancode == InvalidNativeKeycode())
-    return;
-
   // Windows scancodes are only 8-bit, so store the low-order byte into the
   // event and set the extended flag if any high-order bits are set. The only
   // high-order values we should see are 0xE0 or 0xE1. The extended bit usually
   // distinguishes keys with the same meaning, e.g. left & right shift.
   input.ki.wScan = scancode & 0xFF;
-  if ((scancode & 0xFF00) != 0x0000) {
+  if ((scancode & 0xFF00) != 0x0000)
     input.ki.dwFlags |= KEYEVENTF_EXTENDEDKEY;
-  }
 
-  if (SendInput(1, &input, sizeof(INPUT)) == 0) {
+  if (SendInput(1, &input, sizeof(INPUT)) == 0)
     LOG_GETLASTERROR(ERROR) << "Failed to inject a key event";
-  }
 }
 
 void EventExecutorWin::Core::HandleMouse(const MouseEvent& event) {
@@ -236,9 +237,8 @@ void EventExecutorWin::Core::HandleMouse(const MouseEvent& event) {
       input.mi.dy = static_cast<int>((y * 65535) / (screen_size.height() - 1));
       input.mi.dwFlags =
           MOUSEEVENTF_MOVE | MOUSEEVENTF_ABSOLUTE | MOUSEEVENTF_VIRTUALDESK;
-      if (SendInput(1, &input, sizeof(INPUT)) == 0) {
+      if (SendInput(1, &input, sizeof(INPUT)) == 0)
         LOG_GETLASTERROR(ERROR) << "Failed to inject a mouse move event";
-      }
     }
   }
 
@@ -257,16 +257,14 @@ void EventExecutorWin::Core::HandleMouse(const MouseEvent& event) {
     if (wheel_delta_x != 0) {
       wheel.mi.mouseData = wheel_delta_x;
       wheel.mi.dwFlags = MOUSEEVENTF_HWHEEL;
-      if (SendInput(1, &wheel, sizeof(INPUT)) == 0) {
+      if (SendInput(1, &wheel, sizeof(INPUT)) == 0)
         LOG_GETLASTERROR(ERROR) << "Failed to inject a mouse wheel(x) event";
-      }
     }
     if (wheel_delta_y != 0) {
       wheel.mi.mouseData = wheel_delta_y;
       wheel.mi.dwFlags = MOUSEEVENTF_WHEEL;
-      if (SendInput(1, &wheel, sizeof(INPUT)) == 0) {
+      if (SendInput(1, &wheel, sizeof(INPUT)) == 0)
         LOG_GETLASTERROR(ERROR) << "Failed to inject a mouse wheel(y) event";
-      }
     }
   }
 
@@ -293,9 +291,8 @@ void EventExecutorWin::Core::HandleMouse(const MouseEvent& event) {
           down ? MOUSEEVENTF_LEFTDOWN : MOUSEEVENTF_LEFTUP;
     }
 
-    if (SendInput(1, &button_event, sizeof(INPUT)) == 0) {
+    if (SendInput(1, &button_event, sizeof(INPUT)) == 0)
       LOG_GETLASTERROR(ERROR) << "Failed to inject a mouse button event";
-    }
   }
 }
 
diff --git a/remoting/host/linux/x_server_clipboard.cc b/remoting/host/linux/x_server_clipboard.cc
index b4e7d63..230d93b6 100644
--- a/remoting/host/linux/x_server_clipboard.cc
+++ b/remoting/host/linux/x_server_clipboard.cc
@@ -10,6 +10,7 @@
 #include "base/callback.h"
 #include "base/logging.h"
 #include "remoting/base/constants.h"
+#include "remoting/base/util.h"
 
 namespace remoting {
 
@@ -90,12 +91,14 @@ void XServerClipboard::SetClipboard(const std::string& mime_type,
                                     const std::string& data) {
   DCHECK(display_);
 
-  if (clipboard_window_ == BadValue) {
+  if (clipboard_window_ == BadValue)
     return;
-  }
 
   // Currently only UTF-8 is supported.
-  if (mime_type != kMimeTypeTextUtf8) {
+  if (mime_type != kMimeTypeTextUtf8)
+    return;
+  if (!StringIsUtf8(data.c_str(), data.length())) {
+    LOG(ERROR) << "ClipboardEvent: data is not UTF-8 encoded.";
     return;
   }
 
@@ -151,15 +154,13 @@ void XServerClipboard::OnSetSelectionOwnerNotify(Atom selection,
     return;
   }
 
-  if (selection != clipboard_atom_ && selection != XA_PRIMARY) {
-    // Only process PRIMARY and CLIPBOARD selections.
+  // Only process PRIMARY and CLIPBOARD selections.
+  if (selection != clipboard_atom_ && selection != XA_PRIMARY)
     return;
-  }
 
   // If we own the selection, don't request details for it.
-  if (IsSelectionOwner(selection)) {
+  if (IsSelectionOwner(selection))
     return;
-  }
 
   get_selections_time_ = base::TimeTicks::Now();
 
@@ -185,9 +186,8 @@ void XServerClipboard::OnPropertyNotify(XEvent* event) {
       XFree(data);
 
       // If the property is zero-length then the large transfer is complete.
-      if (item_count == 0) {
+      if (item_count == 0)
         large_selection_property_ = None;
-      }
     }
   }
 }
@@ -227,9 +227,8 @@ void XServerClipboard::OnSelectionRequest(XEvent* event) {
   selection_event.selection = event->xselectionrequest.selection;
   selection_event.time = event->xselectionrequest.time;
   selection_event.target = event->xselectionrequest.target;
-  if (event->xselectionrequest.property == None) {
+  if (event->xselectionrequest.property == None)
     event->xselectionrequest.property = event->xselectionrequest.target;
-  }
   if (!IsSelectionOwner(selection_event.selection)) {
     selection_event.property = None;
   } else {
@@ -303,9 +302,8 @@ void XServerClipboard::HandleSelectionNotify(XSelectionEvent* event,
     finished = HandleSelectionStringEvent(event, format, item_count, data);
   }
 
-  if (finished) {
+  if (finished)
     get_selections_time_ = base::TimeTicks();
-  }
 }
 
 bool XServerClipboard::HandleSelectionTargetsEvent(XSelectionEvent* event,
@@ -337,15 +335,14 @@ bool XServerClipboard::HandleSelectionStringEvent(XSelectionEvent* event,
                                                   int format,
                                                   int item_count,
                                                   void* data) {
-  if (event->property != selection_string_atom_ || !data || format != 8) {
+  if (event->property != selection_string_atom_ || !data || format != 8)
     return true;
-  }
 
   std::string text(static_cast<char*>(data), item_count);
 
-  if (event->target == XA_STRING || event->target == utf8_string_atom_) {
+  if (event->target == XA_STRING || event->target == utf8_string_atom_)
     NotifyClipboardText(text);
-  }
+
   return true;
 }
 
diff --git a/remoting/protocol/clipboard_stub.h b/remoting/protocol/clipboard_stub.h
index 2cc5051..e15f44a 100644
--- a/remoting/protocol/clipboard_stub.h
+++ b/remoting/protocol/clipboard_stub.h
@@ -20,6 +20,8 @@ class ClipboardStub {
   ClipboardStub() {}
   virtual ~ClipboardStub() {}
 
+  // Implementations must not assume the presence of |event|'s fields, nor that
+  // |event.data| is correctly encoded according to the specified MIME-type.
   virtual void InjectClipboardEvent(const ClipboardEvent& event) = 0;
 
  private:
diff --git a/remoting/protocol/input_stub.h b/remoting/protocol/input_stub.h
index 121ca98..9183ae0 100644
--- a/remoting/protocol/input_stub.h
+++ b/remoting/protocol/input_stub.h
@@ -21,6 +21,8 @@ class InputStub {
   InputStub() {}
   virtual ~InputStub() {}
 
+  // Implementations must never assume the presence of any |event| fields,
+  // nor assume that their contents are valid.
   virtual void InjectKeyEvent(const KeyEvent& event) = 0;
   virtual void InjectMouseEvent(const MouseEvent& event) = 0;