DesktopSessionAgent now hosts the video capturer and provides necessary plumbing to drive it via an…

remoting/host/desktop_session_agent.cc

Index: remoting/host/desktop_session_agent.cc
diff --git a/remoting/host/desktop_session_agent.cc b/remoting/host/desktop_session_agent.cc
index 36643695a99f0b5360ffd63b0154fa1b3549cc31..ac6777799019ca871298c50b95e6b14b2935ef81 100644
--- a/remoting/host/desktop_session_agent.cc
+++ b/remoting/host/desktop_session_agent.cc
@@ -9,27 +9,35 @@
 #include "ipc/ipc_message.h"
 #include "ipc/ipc_message_macros.h"
 #include "remoting/base/auto_thread_task_runner.h"
+#include "remoting/base/capture_data.h"
 #include "remoting/host/chromoting_messages.h"
+#include "remoting/proto/control.pb.h"
 
 namespace remoting {
 
 DesktopSessionAgent::~DesktopSessionAgent() {
-  DCHECK(caller_task_runner()->BelongsToCurrentThread());
+  DCHECK(!video_capturer_);
 }
 
 bool DesktopSessionAgent::OnMessageReceived(const IPC::Message& message) {
   DCHECK(caller_task_runner()->BelongsToCurrentThread());
 
-  NOTIMPLEMENTED();
-  return false;
+  bool handled = true;
+  IPC_BEGIN_MESSAGE_MAP(DesktopSessionAgent, message)
+    IPC_MESSAGE_HANDLER(ChromotingNetworkDesktopMsg_CaptureFrame,
+                        OnCaptureFrame)
+    IPC_MESSAGE_HANDLER(ChromotingNetworkDesktopMsg_InvalidateRegion,
+                        OnInvalidateRegion)
+    IPC_MESSAGE_HANDLER(ChromotingNetworkDesktopMsg_SharedBufferCreated,
+                        OnSharedBufferCreated)
+  IPC_END_MESSAGE_MAP()
+  return handled;
 }
 
 void DesktopSessionAgent::OnChannelConnected(int32 peer_pid) {
   DCHECK(caller_task_runner()->BelongsToCurrentThread());
 
   VLOG(1) << "IPC: desktop <- network (" << peer_pid << ")";
-
-  NOTIMPLEMENTED();
 }
 
 void DesktopSessionAgent::OnChannelError() {
@@ -38,23 +46,174 @@ void DesktopSessionAgent::OnChannelError() {
   // Make sure the channel is closed.
   network_channel_.reset();
 
-  // Notify the caller that |this| can be destroyed now.
-  done_task_.Run();
+  // Notify the caller that the channel has been disconnected.
+  disconnected_task_.Run();
+}
+
+scoped_refptr<SharedBuffer> DesktopSessionAgent::CreateSharedBuffer(
+    uint32 size) {
+  DCHECK(video_capture_task_runner()->BelongsToCurrentThread());
+
+  scoped_refptr<SharedBuffer> buffer = new SharedBuffer(size);
+  if (buffer->ptr() != NULL) {
+    // Use buffer's address as its ID.
+    buffer->set_id(reinterpret_cast<intptr_t>(buffer.get()));

[Cris Neckar, 2012/11/20 22:23:49]

In practice this may not be an issue but it is generally a really bad idea to
use the address as an ID across security boundaries. If this is exposed to
anything untrusted it will break ASLR. This would be especially bad if these IDs
are ever exposed to javascript or the web. Can you give me some more context on
where this id is relevant or accessible from?

[alexeypa (please no reviews), 2012/11/20 22:56:03]

There are two processes: desktop and network. The desktop process runs as
SYSTEM. The network process runs at lower privileges. The network process is
protected by a security descriptor allowing access to SYSTEM only. Both are not
loading any 3rd party code (except of DLLs injected by anti viruses and other
privileged software/malware, of course).

The pipe between two processes is creates and connected by code running under
SYSTEM. The client end of the connected pipe is passed to the network process.

The IDs are created by the desktop process and passed to the network process one
way. An ID is passed along with a handle of a shared section created by the
desktop process.

At the moment the network process blindly trusts any ID and other message data
passed by the desktop process. If the desktop process passes an invalid message
the network process will likely crash. 

[Cris Neckar, 2012/11/21 01:00:54]

Gotcha, if it's doable can you use something other than a pointer as the ID
here. I don't see any reason why it would need to be the pointer. We prefer to
never pass pointers across IPCs (even if they aren't used as pointers).

[alexeypa (please no reviews), 2012/11/22 00:59:24]

Done.

+    shared_buffers_.push_back(buffer);
+
+    SendToNetwork(new ChromotingDesktopNetworkMsg_CreateSharedBuffer(
+        buffer->id(), buffer->handle(), buffer->size()));
+  }
+
+  return buffer;
+}
+
+void DesktopSessionAgent::ReleaseSharedBuffer(
+    scoped_refptr<SharedBuffer> buffer) {
+  DCHECK(video_capture_task_runner()->BelongsToCurrentThread());
+  DCHECK(buffer->id() != 0);
+
+  SendToNetwork(
+      new ChromotingDesktopNetworkMsg_ReleaseSharedBuffer(buffer->id()));
 }
 
-bool DesktopSessionAgent::Start(const base::Closure& done_task,
+void DesktopSessionAgent::OnCaptureCompleted(
+    scoped_refptr<CaptureData> capture_data) {
+  DCHECK(video_capture_task_runner()->BelongsToCurrentThread());
+
+  // Serialize CaptureData
+  SerializedCapturedData serialized_data;
+  serialized_data.shared_buffer_id = capture_data->shared_buffer()->id();
+  serialized_data.dimensions = capture_data->size();
+  serialized_data.pixel_format = capture_data->pixel_format();
+  serialized_data.capture_time_ms = capture_data->capture_time_ms();
+  serialized_data.client_sequence_number =
+      capture_data->client_sequence_number();
+  serialized_data.dpi = capture_data->dpi();
+  for (SkRegion::Iterator i(capture_data->dirty_region()); !i.done(); i.next())
+    serialized_data.dirty_region.push_back(i.rect());
+
+  SendToNetwork(
+      new ChromotingDesktopNetworkMsg_CaptureCompleted(serialized_data));
+}
+
+void DesktopSessionAgent::OnCursorShapeChanged(
+    scoped_ptr<protocol::CursorShapeInfo> cursor_shape) {
+  DCHECK(video_capture_task_runner()->BelongsToCurrentThread());
+
+  NOTIMPLEMENTED();
+}
+
+bool DesktopSessionAgent::Start(const base::Closure& disconnected_task,
                                 IPC::PlatformFileForTransit* desktop_pipe_out) {
   DCHECK(caller_task_runner()->BelongsToCurrentThread());
 
-  done_task_ = done_task;
-  return DoCreateNetworkChannel(desktop_pipe_out, &network_channel_);
+  disconnected_task_ = disconnected_task;
+  if (!CreateChannelForNetworkProcess(desktop_pipe_out, &network_channel_))
+    return false;
+
+  // Start the video capturer.
+  video_capture_task_runner()->PostTask(
+      FROM_HERE, base::Bind(&DesktopSessionAgent::StartVideoCapturer, this));
+  return true;
+}
+
+void DesktopSessionAgent::Stop() {
+  DCHECK(caller_task_runner()->BelongsToCurrentThread());
+
+  // Stop the video capturer.
+  video_capture_task_runner()->PostTask(
+      FROM_HERE, base::Bind(&DesktopSessionAgent::StopVideoCapturer, this));
+}
+
+void DesktopSessionAgent::InvalidateRegion(
+    scoped_ptr<SkRegion> invalid_region) {
+  DCHECK(video_capture_task_runner()->BelongsToCurrentThread());
+
+  video_capturer_->InvalidateRegion(*invalid_region);
+}
+
+void DesktopSessionAgent::OnCaptureFrame() {
+  if (!video_capture_task_runner()->BelongsToCurrentThread()) {
+    video_capture_task_runner()->PostTask(
+        FROM_HERE,
+        base::Bind(&DesktopSessionAgent::OnCaptureFrame, this));
+    return;
+  }
+
+  video_capturer_->CaptureInvalidRegion();
+}
+
+void DesktopSessionAgent::OnInvalidateRegion(
+    const std::vector<SkIRect>& invalid_rects) {

[aedla, 2012/11/21 21:53:54]

I can't find any check that invalid_rects has sane values. And it looks like the
attacker controlled fLeft, fRight, fTop, fBottom could reach code like:

VideoFrameCapturerLinux::FastBlit():
  int dst_x = rect.fLeft, dst_y = rect.fTop;
  ...
  uint8* dst_pos = dst_buffer + dst_stride * dst_y;
  ...
  memcpy(dst_pos, src_pos, row_bytes);

or

VideoEncoderVerbatim::EncodeRect():
  const uint8* in = capture_data_->data_planes().data[0] +
      rect.fTop * strides + rect.fLeft * bytes_per_pixel;
  ...
  memcpy(out + filled, in + row_pos, bytes_to_copy);

[alexeypa (please no reviews), 2012/11/22 00:59:24]

Done. The list of rects is sanitized now.

+  DCHECK(caller_task_runner()->BelongsToCurrentThread());
+
+  scoped_ptr<SkRegion> invalid_region(new SkRegion());
+  invalid_region->setRects(&invalid_rects[0], invalid_rects.size());
+
+  video_capture_task_runner()->PostTask(
+      FROM_HERE, base::Bind(&DesktopSessionAgent::InvalidateRegion, this,
+                            base::Passed(&invalid_region)));
+}
+
+void DesktopSessionAgent::OnSharedBufferCreated(intptr_t id) {
+  if (!video_capture_task_runner()->BelongsToCurrentThread()) {
+    video_capture_task_runner()->PostTask(
+        FROM_HERE,
+        base::Bind(&DesktopSessionAgent::OnSharedBufferCreated, this, id));
+    return;
+  }
+
+  // Drop the cached reference to the buffer.
+  SharedBuffers::iterator i = shared_buffers_.begin();
+  for (; i != shared_buffers_.end(); ++i) {
+    if ((*i)->id() == id) {
+      shared_buffers_.erase(i);
+      break;
+    }
+  }
+}
+
+void DesktopSessionAgent::SendToNetwork(IPC::Message* message) {
+  if (!caller_task_runner()->BelongsToCurrentThread()) {
+    caller_task_runner()->PostTask(
+        FROM_HERE,
+        base::Bind(&DesktopSessionAgent::SendToNetwork, this, message));
+    return;
+  }
+
+  if (network_channel_) {
+    network_channel_->Send(message);
+  } else {
+    delete message;
+  }
+}
+
+void DesktopSessionAgent::StartVideoCapturer() {
+  DCHECK(video_capture_task_runner()->BelongsToCurrentThread());
+
+  video_capturer_ = VideoFrameCapturer::CreateWithFactory(this);
+  video_capturer_->Start(this);
+}
+
+void DesktopSessionAgent::StopVideoCapturer() {
+  DCHECK(video_capture_task_runner()->BelongsToCurrentThread());
+
+  if (video_capturer_) {
+    video_capturer_->Stop();
+    video_capturer_.reset();
+  }
+
+  // Free any shared buffers left.
+  shared_buffers_.clear();
 }
 
 DesktopSessionAgent::DesktopSessionAgent(
     scoped_refptr<AutoThreadTaskRunner> caller_task_runner,
-    scoped_refptr<AutoThreadTaskRunner> io_task_runner)
+    scoped_refptr<AutoThreadTaskRunner> io_task_runner,
+    scoped_refptr<AutoThreadTaskRunner> video_capture_task_runner)
     : caller_task_runner_(caller_task_runner),
-      io_task_runner_(io_task_runner) {
+      io_task_runner_(io_task_runner),
+      video_capture_task_runner_(video_capture_task_runner) {
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
 }