Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11413022:
DesktopSessionAgent now hosts the video capturer and provides necessary plumbing to drive it via an… (Closed)
|
Created:
8 years, 1 month ago by alexeypa (please no reviews) Modified:
8 years ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, sail+watch_chromium.org, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, rmsousa+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDesktopSessionAgent now hosts the video capturer and provides necessary plumbing to drive it via an IPC channel.
BUG=134694
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=169587
Patch Set 1 #
Total comments: 57
Patch Set 2 : CR feedback & rebased on top of https://chromiumcodereview.appspot.com/11416021/ #
Total comments: 4
Patch Set 3 : CR feedback. #Patch Set 4 : rebased #
Total comments: 6
Patch Set 5 : Security CR feedback. #
Total comments: 2
Patch Set 6 : rebased #Patch Set 7 : Posix. #Patch Set 8 : fixing clang compilation. #
Messages
Total messages: 25 (0 generated)
PTAL.
https://codereview.chromium.org/11413022/diff/1/remoting/host/chromoting_mess... File remoting/host/chromoting_messages.h (right): https://codereview.chromium.org/11413022/diff/1/remoting/host/chromoting_mess... remoting/host/chromoting_messages.h:6: #define REMOTING_HOST_CHROMOTING_MESSAGES_H_ nit: Chromoting IPC messages, for clarity? https://codereview.chromium.org/11413022/diff/1/remoting/host/chromoting_mess... remoting/host/chromoting_messages.h:100: IPC_MESSAGE_CONTROL3(ChromotingDesktopNetworkMsg_RegisterSharedBuffer, nit: Make this & next fn Add/RemoveSharedBuffer? https://codereview.chromium.org/11413022/diff/1/remoting/host/chromoting_mess... remoting/host/chromoting_messages.h:134: // Dimentions of the buffer in pixels. typo: dimensions https://codereview.chromium.org/11413022/diff/1/remoting/host/chromoting_mess... remoting/host/chromoting_messages.h:137: // Pixel format. nit: "Format of the shared pixel data buffer."? https://codereview.chromium.org/11413022/diff/1/remoting/host/chromoting_mess... remoting/host/chromoting_messages.h:141: IPC_STRUCT_MEMBER(int, capture_time_ms) nit: I'd suggest moving this to the end of the struct and noting in the comment that it's for debug purposes. https://codereview.chromium.org/11413022/diff/1/remoting/host/chromoting_mess... remoting/host/chromoting_messages.h:160: // the network process at any time. Do you mean it can be safely dropped by the desktop process at any time? What does it mean for the process to "drop" it; you mean to invalidate the previously-shared handle? What if the network process can't map the buffer for some reason? https://codereview.chromium.org/11413022/diff/1/remoting/host/chromoting_mess... remoting/host/chromoting_messages.h:167: IPC_MESSAGE_CONTROL0(ChromotingNetworkDesktopMsg_CaptureFrame) nit: Add comments to explain this & InvalidateRegion. https://codereview.chromium.org/11413022/diff/1/remoting/host/desktop_process.cc File remoting/host/desktop_process.cc (right): https://codereview.chromium.org/11413022/diff/1/remoting/host/desktop_process... remoting/host/desktop_process.cc:63: caller_task_runner_ = NULL; Do you really need to NULL |caller_task_runner_| here? https://codereview.chromium.org/11413022/diff/1/remoting/host/desktop_process... remoting/host/desktop_process.cc:90: desktop_agent_ = NULL; Is there definitely nothing in Stop() that needs doing in this case? https://codereview.chromium.org/11413022/diff/1/remoting/host/desktop_session... File remoting/host/desktop_session_agent.cc (right): https://codereview.chromium.org/11413022/diff/1/remoting/host/desktop_session... remoting/host/desktop_session_agent.cc:58: // Use buffer's address as its ID. Do we care that that discloses information about the agent's memory layout to a potentially less privileged process? https://codereview.chromium.org/11413022/diff/1/remoting/host/desktop_session... remoting/host/desktop_session_agent.cc:114: void DesktopSessionAgent::Stop() { Thread check? https://codereview.chromium.org/11413022/diff/1/remoting/host/desktop_session... remoting/host/desktop_session_agent.cc:139: const std::vector<SkIRect>& invalid_region) { nit: invalid_rects or region_rects https://codereview.chromium.org/11413022/diff/1/remoting/host/desktop_session... remoting/host/desktop_session_agent.cc:147: base::Passed(®ion))); Hmmmm; this is the sort of glue I'd hoped to avoid, by having the caller call VideoFrameCapturer on whatever thread they like, and have the capturer switch to the capture thread, and back, if necessary. https://codereview.chromium.org/11413022/diff/1/remoting/host/desktop_session... remoting/host/desktop_session_agent.cc:168: void DesktopSessionAgent::SendToNetwork(IPC::Message* message) { nit: SendIpcToNetwork? https://codereview.chromium.org/11413022/diff/1/remoting/host/desktop_session... remoting/host/desktop_session_agent.cc:195: video_capturer_->UseSharedBuffers(NULL); Why do you need to NULL the buffer allocator / sender here? https://codereview.chromium.org/11413022/diff/1/remoting/host/desktop_session... File remoting/host/desktop_session_agent.h (right): https://codereview.chromium.org/11413022/diff/1/remoting/host/desktop_session... remoting/host/desktop_session_agent.h:55: // VideoFrameCapturer::Delegate implementation. We should follow up to re-name VideoFrameCapturer::Delegate, since it's not a delegate. https://codereview.chromium.org/11413022/diff/1/remoting/host/desktop_session... remoting/host/desktop_session_agent.h:63: // pipe to be forwarder to the corresponding desktop environment. typo: forwarded. I'd suggest rewording this to make it clearer that this function creates the desktop-integration components, then creates a pipe over which to receive IPCs for them, returning the client end to pass to the network process. Why do we have separate |done_task| and |desktop_pipe_out|, rather than a single callback that includes |desktop_pipe_out|? The comment should also explain what |done_task| is and how it relates to the bool return value. https://codereview.chromium.org/11413022/diff/1/remoting/host/desktop_session... remoting/host/desktop_session_agent.h:67: // Stops the agent asynchronously. How does the caller know when this object is completely destroyed? Or does it not care? Based on the comment on |done_task|, it sounds like this method should take |done_task|, not Start? https://codereview.chromium.org/11413022/diff/1/remoting/host/desktop_session... remoting/host/desktop_session_agent.h:79: // Creates a pre-connected IPC channel to be used to access the screen/audio nit: What does "pre-connected" mean? Do you just mean "connected"? https://codereview.chromium.org/11413022/diff/1/remoting/host/desktop_session... remoting/host/desktop_session_agent.h:81: virtual bool DoCreateNetworkChannel( nit: CreateChannelForNetworkProcess or similar? https://codereview.chromium.org/11413022/diff/1/remoting/host/desktop_session... remoting/host/desktop_session_agent.h:85: void InvalidateRegion(scoped_ptr<SkRegion> invalid_region); Why not fold this in to OnInvalidateRegion? https://codereview.chromium.org/11413022/diff/1/remoting/host/desktop_session... remoting/host/desktop_session_agent.h:87: // Implements VideoFrameCapturer::CaptureInvalidRegion. nit: You mean "Implements the CaptureInvalidRegion IPC." or "Handles CaptureInvalidRegion requests from the client." I think? https://codereview.chromium.org/11413022/diff/1/remoting/host/desktop_session... remoting/host/desktop_session_agent.h:101: void StopVideoCapturer(); nit: Add comments to explain the function of these. https://codereview.chromium.org/11413022/diff/1/remoting/host/desktop_session... remoting/host/desktop_session_agent.h:103: scoped_refptr<AutoThreadTaskRunner> caller_task_runner() const { Add a comment explaining that these getters provide platform-specific derived classes access to the task runners. https://codereview.chromium.org/11413022/diff/1/remoting/host/desktop_session... remoting/host/desktop_session_agent.h:122: // Task runner on which video public methods of this class should be called. Is this correct? Isn't this the thread on which this object will call out to the video capturer?
PTAL. https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/chromoti... File remoting/host/chromoting_messages.h (right): https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/chromoti... remoting/host/chromoting_messages.h:6: #define REMOTING_HOST_CHROMOTING_MESSAGES_H_ On 2012/11/16 23:37:31, Wez wrote: > nit: Chromoting IPC messages, for clarity? No the file name is chromoting_messages.h. Other IPC message files are also called xxx_messages.h, so I'll follow the pattern. https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/chromoti... remoting/host/chromoting_messages.h:100: IPC_MESSAGE_CONTROL3(ChromotingDesktopNetworkMsg_RegisterSharedBuffer, On 2012/11/16 23:37:31, Wez wrote: > nit: Make this & next fn Add/RemoveSharedBuffer? Done. https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/chromoti... remoting/host/chromoting_messages.h:134: // Dimentions of the buffer in pixels. On 2012/11/16 23:37:31, Wez wrote: > typo: dimensions Done. https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/chromoti... remoting/host/chromoting_messages.h:137: // Pixel format. On 2012/11/16 23:37:31, Wez wrote: > nit: "Format of the shared pixel data buffer."? Why? The shared buffer does not know it carries pixels. Pixel format is local to this message. It is clear what pixels it is talking about since the whole message is about describing an image buffer. https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/chromoti... remoting/host/chromoting_messages.h:141: IPC_STRUCT_MEMBER(int, capture_time_ms) On 2012/11/16 23:37:31, Wez wrote: > nit: I'd suggest moving this to the end of the struct and noting in the comment > that it's for debug purposes. Right now the order of fields in this structure matches the order in CaptureData. The value carried by |capture_time_ms| is used when scheduling next frame capture, not only for debugging. There is a follow up CL in my queue that fixes calculation of |capture_time_ms| value and takes advantage of this field. https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/chromoti... remoting/host/chromoting_messages.h:160: // the network process at any time. On 2012/11/16 23:37:31, Wez wrote: > Do you mean it can be safely dropped by the desktop process at any time? What > does it mean for the process to "drop" it; you mean to invalidate the > previously-shared handle? It means the process can safely unmap the memory and close the handle. The desktop process should have created a local mapping on the buffer and local copy of the handle by this time. > What if the network process can't map the buffer for > some reason? It is considered a failure. Right now captures involving this buffer will return NULL CaptureData pointer. Yes, it needs to be handled better. https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/chromoti... remoting/host/chromoting_messages.h:167: IPC_MESSAGE_CONTROL0(ChromotingNetworkDesktopMsg_CaptureFrame) On 2012/11/16 23:37:31, Wez wrote: > nit: Add comments to explain this & InvalidateRegion. Done. https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... File remoting/host/desktop_process.cc (right): https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... remoting/host/desktop_process.cc:63: caller_task_runner_ = NULL; On 2012/11/16 23:37:31, Wez wrote: > Do you really need to NULL |caller_task_runner_| here? Yes. This is the way to stop the process. https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... remoting/host/desktop_process.cc:90: desktop_agent_ = NULL; On 2012/11/16 23:37:31, Wez wrote: > Is there definitely nothing in Stop() that needs doing in this case? It is all done by DesktopProcess::OnChannelError(). https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... File remoting/host/desktop_session_agent.cc (right): https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... remoting/host/desktop_session_agent.cc:58: // Use buffer's address as its ID. On 2012/11/16 23:37:31, Wez wrote: > Do we care that that discloses information about the agent's memory layout to a > potentially less privileged process? I don't see how it can be exploited. https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... remoting/host/desktop_session_agent.cc:114: void DesktopSessionAgent::Stop() { On 2012/11/16 23:37:31, Wez wrote: > Thread check? Done. https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... remoting/host/desktop_session_agent.cc:139: const std::vector<SkIRect>& invalid_region) { On 2012/11/16 23:37:31, Wez wrote: > nit: invalid_rects or region_rects Done. https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... remoting/host/desktop_session_agent.cc:147: base::Passed(®ion))); On 2012/11/16 23:37:31, Wez wrote: > Hmmmm; this is the sort of glue I'd hoped to avoid, by having the caller call > VideoFrameCapturer on whatever thread they like, and have the capturer switch to > the capture thread, and back, if necessary. VideoFrameCapturer is designed to run on a single thread. This CL is not a good place to change that. https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... remoting/host/desktop_session_agent.cc:168: void DesktopSessionAgent::SendToNetwork(IPC::Message* message) { On 2012/11/16 23:37:31, Wez wrote: > nit: SendIpcToNetwork? I don't think so: 1. There are other SendToXxx wrappers in the code base. SendIpcXxx will not be consistent. 2. It is too verbose, especially because the compiler is checking the parameter's type. https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... remoting/host/desktop_session_agent.cc:195: video_capturer_->UseSharedBuffers(NULL); On 2012/11/16 23:37:31, Wez wrote: > Why do you need to NULL the buffer allocator / sender here? Done. https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... File remoting/host/desktop_session_agent.h (right): https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... remoting/host/desktop_session_agent.h:55: // VideoFrameCapturer::Delegate implementation. On 2012/11/16 23:37:31, Wez wrote: > We should follow up to re-name VideoFrameCapturer::Delegate, since it's not a > delegate. Ack. https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... remoting/host/desktop_session_agent.h:63: // pipe to be forwarder to the corresponding desktop environment. On 2012/11/16 23:37:31, Wez wrote: > typo: forwarded. > I'd suggest rewording this to make it clearer that this function creates the > desktop-integration components, then creates a pipe over which to receive IPCs > for them, returning the client end to pass to the network process. Done. > Why do we have separate |done_task| and |desktop_pipe_out|, rather than a single > callback that includes |desktop_pipe_out|? |done_task| is not part of starting/stopping of this object. > The comment should also explain what |done_task| is and how it relates to the > bool return value. Added a comment and renamed |done_task| to |disconnect_task| to avoid this confusion. https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... remoting/host/desktop_session_agent.h:67: // Stops the agent asynchronously. On 2012/11/16 23:37:31, Wez wrote: > How does the caller know when this object is completely destroyed? Or does it > not care? Based on the comment on |done_task|, it sounds like this method > should take |done_task|, not Start? I renamed |done_task| so it is not getting associated with Start() and Stop(). The caller does not care when |this| is destroyed. https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... remoting/host/desktop_session_agent.h:79: // Creates a pre-connected IPC channel to be used to access the screen/audio On 2012/11/16 23:37:31, Wez wrote: > nit: What does "pre-connected" mean? Do you just mean "connected"? Done. https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... remoting/host/desktop_session_agent.h:81: virtual bool DoCreateNetworkChannel( On 2012/11/16 23:37:31, Wez wrote: > nit: CreateChannelForNetworkProcess or similar? Done. https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... remoting/host/desktop_session_agent.h:85: void InvalidateRegion(scoped_ptr<SkRegion> invalid_region); On 2012/11/16 23:37:31, Wez wrote: > Why not fold this in to OnInvalidateRegion? To avoid creating a copy of a bunch of rects. OnInvalidateRegion converts std::vector<SkIRect> into SkRegion allocated on heap. If I defer this conversion until OnInvalidateRegion is running on video_capture_task_runner() an additional copy of the vector will be created. https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... remoting/host/desktop_session_agent.h:87: // Implements VideoFrameCapturer::CaptureInvalidRegion. On 2012/11/16 23:37:31, Wez wrote: > nit: You mean "Implements the CaptureInvalidRegion IPC." or "Handles > CaptureInvalidRegion requests from the client." I think? Done. https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... remoting/host/desktop_session_agent.h:101: void StopVideoCapturer(); On 2012/11/16 23:37:31, Wez wrote: > nit: Add comments to explain the function of these. Done. What there methods do is documented by their names, so I'm not sure how useful these comments are. https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... remoting/host/desktop_session_agent.h:103: scoped_refptr<AutoThreadTaskRunner> caller_task_runner() const { On 2012/11/16 23:37:31, Wez wrote: > Add a comment explaining that these getters provide platform-specific derived > classes access to the task runners. I don't see any value in such a comment. There are getters which is obvious from their naming and inline nature. They are defined in protected section, so it is fairly obvious that only derived classes can use them. https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... remoting/host/desktop_session_agent.h:122: // Task runner on which video public methods of this class should be called. On 2012/11/16 23:37:31, Wez wrote: > Is this correct? Isn't this the thread on which this object will call out to the > video capturer? Done.
LGTM w/ nits https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... File remoting/host/desktop_process.cc (right): https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... remoting/host/desktop_process.cc:63: caller_task_runner_ = NULL; On 2012/11/19 21:46:25, alexeypa wrote: > On 2012/11/16 23:37:31, Wez wrote: > > Do you really need to NULL |caller_task_runner_| here? > > Yes. This is the way to stop the process. But if you NULL it then caller_task_runner_->BelongsToCurrentThread checks will crash, so I'd generally been trying to avoid that in classes. On the face if it it looks OK in this case, though. https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... remoting/host/desktop_process.cc:90: desktop_agent_ = NULL; On 2012/11/19 21:46:25, alexeypa wrote: > On 2012/11/16 23:37:31, Wez wrote: > > Is there definitely nothing in Stop() that needs doing in this case? > > It is all done by DesktopProcess::OnChannelError(). Could you do without NULLing |desktop_agent_| here, as well, since it's NULLed in OnChannelError? https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... File remoting/host/desktop_session_agent.h (right): https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... remoting/host/desktop_session_agent.h:101: void StopVideoCapturer(); On 2012/11/19 21:46:25, alexeypa wrote: > On 2012/11/16 23:37:31, Wez wrote: > > nit: Add comments to explain the function of these. > > Done. What there methods do is documented by their names, so I'm not sure how > useful these comments are. You only need these methods so you have something to bind to post to the capture thread, so I'd suggest "Posted to the capture thread to start the capturer", for example. https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... remoting/host/desktop_session_agent.h:103: scoped_refptr<AutoThreadTaskRunner> caller_task_runner() const { On 2012/11/19 21:46:25, alexeypa wrote: > On 2012/11/16 23:37:31, Wez wrote: > > Add a comment explaining that these getters provide platform-specific derived > > classes access to the task runners. > > I don't see any value in such a comment. There are getters which is obvious from > their naming and inline nature. They are defined in protected section, so it is > fairly obvious that only derived classes can use them. Right; a comment should describe why we need them, not what they are, i.e. clarify that they're here for derived classes to use, not just something that got missed in a refactoring. :) https://chromiumcodereview.appspot.com/11413022/diff/5002/remoting/host/deskt... File remoting/host/desktop_session_agent.h (right): https://chromiumcodereview.appspot.com/11413022/diff/5002/remoting/host/deskt... remoting/host/desktop_session_agent.h:64: // |stopped_task| is invoked on |caller_task_runner_| to notify the caller nit: Update this comment. https://chromiumcodereview.appspot.com/11413022/diff/5002/remoting/host/deskt... remoting/host/desktop_session_agent.h:89: // Handles CaptureInvalidRegion requests from the client. nit: Update this comment.
https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... File remoting/host/desktop_process.cc (right): https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... remoting/host/desktop_process.cc:90: desktop_agent_ = NULL; On 2012/11/20 07:05:39, Wez wrote: > Could you do without NULLing |desktop_agent_| here, as well, since it's NULLed > in OnChannelError? No, I can't. DesktopProcess::OnChannelError may not be called at all. I added "caller_task_runner_ = NULL;" here as well just to make sure that the message loop will be properly destroyed. https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... File remoting/host/desktop_session_agent.h (right): https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... remoting/host/desktop_session_agent.h:101: void StopVideoCapturer(); On 2012/11/20 07:05:39, Wez wrote: > On 2012/11/19 21:46:25, alexeypa wrote: > > On 2012/11/16 23:37:31, Wez wrote: > > > nit: Add comments to explain the function of these. > > > > Done. What there methods do is documented by their names, so I'm not sure how > > useful these comments are. > > You only need these methods so you have something to bind to post to the capture > thread, so I'd suggest "Posted to the capture thread to start the capturer", for > example. Done. https://chromiumcodereview.appspot.com/11413022/diff/1/remoting/host/desktop_... remoting/host/desktop_session_agent.h:103: scoped_refptr<AutoThreadTaskRunner> caller_task_runner() const { On 2012/11/20 07:05:39, Wez wrote: > Right; a comment should describe why we need them, not what they are, i.e. > clarify that they're here for derived classes to use, not just something that > got missed in a refactoring. :) I added the comment but I still think it is not super useful. https://chromiumcodereview.appspot.com/11413022/diff/5002/remoting/host/deskt... File remoting/host/desktop_session_agent.h (right): https://chromiumcodereview.appspot.com/11413022/diff/5002/remoting/host/deskt... remoting/host/desktop_session_agent.h:64: // |stopped_task| is invoked on |caller_task_runner_| to notify the caller On 2012/11/20 07:05:39, Wez wrote: > nit: Update this comment. Done. https://chromiumcodereview.appspot.com/11413022/diff/5002/remoting/host/deskt... remoting/host/desktop_session_agent.h:89: // Handles CaptureInvalidRegion requests from the client. On 2012/11/20 07:05:39, Wez wrote: > nit: Update this comment. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexeypa@chromium.org/11413022/10002
Presubmit check for 11413022-10002 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
remoting/host/chromoting_messages.h
Presubmit checks took 4.1s to calculate.
cdn@, plase take a look at remoting/host/chromoting_messages.h
Adding cdn@chromium.org this time.
https://codereview.chromium.org/11413022/diff/10002/remoting/host/desktop_ses... File remoting/host/desktop_session_agent.cc (right): https://codereview.chromium.org/11413022/diff/10002/remoting/host/desktop_ses... remoting/host/desktop_session_agent.cc:60: buffer->set_id(reinterpret_cast<intptr_t>(buffer.get())); 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?
https://codereview.chromium.org/11413022/diff/10002/remoting/host/desktop_ses... File remoting/host/desktop_session_agent.cc (right): https://codereview.chromium.org/11413022/diff/10002/remoting/host/desktop_ses... remoting/host/desktop_session_agent.cc:60: buffer->set_id(reinterpret_cast<intptr_t>(buffer.get())); On 2012/11/20 22:23:49, Cris Neckar wrote: > Can you give me some more context on > where this id is relevant or accessible from? 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.
https://chromiumcodereview.appspot.com/11413022/diff/10002/remoting/host/desk... File remoting/host/desktop_session_agent.cc (right): https://chromiumcodereview.appspot.com/11413022/diff/10002/remoting/host/desk... remoting/host/desktop_session_agent.cc:60: buffer->set_id(reinterpret_cast<intptr_t>(buffer.get())); On 2012/11/20 22:56:03, alexeypa wrote: > On 2012/11/20 22:23:49, Cris Neckar wrote: > > Can you give me some more context on > > where this id is relevant or accessible from? > > 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. 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).
https://codereview.chromium.org/11413022/diff/10002/remoting/host/desktop_ses... File remoting/host/desktop_session_agent.cc (right): https://codereview.chromium.org/11413022/diff/10002/remoting/host/desktop_ses... remoting/host/desktop_session_agent.cc:147: const std::vector<SkIRect>& invalid_rects) { 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);
PTAL. https://chromiumcodereview.appspot.com/11413022/diff/10002/remoting/host/desk... File remoting/host/desktop_session_agent.cc (right): https://chromiumcodereview.appspot.com/11413022/diff/10002/remoting/host/desk... remoting/host/desktop_session_agent.cc:60: buffer->set_id(reinterpret_cast<intptr_t>(buffer.get())); On 2012/11/21 01:00:54, Cris Neckar wrote: > On 2012/11/20 22:56:03, alexeypa wrote: > > On 2012/11/20 22:23:49, Cris Neckar wrote: > > > Can you give me some more context on > > > where this id is relevant or accessible from? > > > > 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. > > 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). Done. https://chromiumcodereview.appspot.com/11413022/diff/10002/remoting/host/desk... remoting/host/desktop_session_agent.cc:147: const std::vector<SkIRect>& invalid_rects) { On 2012/11/21 21:53:54, aedla wrote: > 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); Done. The list of rects is sanitized now.
This is a follow up on Cris's question about the attack surface change. This CL introduces the following messages received by a more privileged process: ChromotingNetworkDesktopMsg_SharedBufferCreated(int) ChromotingNetworkDesktopMsg_InvalidateRegion(std::vector<SkIRect>) ChromotingNetworkDesktopMsg_CaptureFrame() Out of the three only ChromotingNetworkDesktopMsg_InvalidateRegion didn't sanitize the parameters as aedla@ has pointed out. This was fixed by patch #5.
lgtm other than one nit https://chromiumcodereview.appspot.com/11413022/diff/7003/remoting/host/deskt... File remoting/host/desktop_session_agent.cc (right): https://chromiumcodereview.appspot.com/11413022/diff/7003/remoting/host/deskt... remoting/host/desktop_session_agent.cc:71: next_shared_buffer_id_ += 2; Nit: If this is already a time consuming operation and this should never happen we might as well put a check here and CRASH() if it ever does occur.
https://chromiumcodereview.appspot.com/11413022/diff/7003/remoting/host/deskt... File remoting/host/desktop_session_agent.cc (right): https://chromiumcodereview.appspot.com/11413022/diff/7003/remoting/host/deskt... remoting/host/desktop_session_agent.cc:71: next_shared_buffer_id_ += 2; On 2012/11/26 22:57:53, Cris Neckar wrote: > Nit: If this is already a time consuming operation and this should never happen > we might as well put a check here and CRASH() if it ever does occur. If we ever reallocate buffers we tend to reallocate all of them, so in wast majority of cases ID overflow is harmless. I think CHECKing would be an overkill.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexeypa@chromium.org/11413022/11
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexeypa@chromium.org/11413022/5007
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexeypa@chromium.org/11413022/4005
Commit queue patch verification failed without an error message. Something went wrong, probably a crash, a hickup or simply the monkeys went out for dinner. Ping the relevant dude on a on-needed basis.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexeypa@chromium.org/11413022/4005
Message was sent while issue was closed.
Change committed as 169587 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
