Fix remoting NM hosts to verify that incoming messages are dictionaries.

Fix remoting NM hosts to verify that incoming messages are dictionaries.

After crrev.com/295851 the PipeMessagingChannel verifies that incoming
messages are dictionaries and then passes them as base::Value to
OnMessage() and then OnMessage() implementation static_cast<> them to
base::DictionaryValue. This is fragile, and also it shouldn't be
PipeMessagingChannel's responsibility to verify format of the messages
it reads. Moved the check to OnMessage() implementations.

Committed: https://crrev.com/c827ad1c743fa8eb5d33f609f6a1e1a317af7c1a
Cr-Commit-Position: refs/heads/master@{#297274}

[Sergey Ulanov, 2014-09-29 19:18:02]

sergeyu@chromium.org changed reviewers:
+ kelvinp@chromium.org

[chromium-reviews, 2014-09-29 19:29:55]

Why not use GetAsDictionary, to keep things type-safe?
On 29 Sep 2014 20:18, <sergeyu@chromium.org> wrote:

> Reviewers: kelvinp,
>
> Description:
> Fix remoting NM hosts to verify that incoming messages are dictionaries.
>
> After crrev.com/295851 the PipeMessagingChannel verifies that incoming
> messages are dictionaries and then passes them as base::Value to
> OnMessage() and then OnMessage() implementation static_cast<> them to
> base::DictionaryValue. This is fragile, and also it shouldn't be
> PipeMessagingChannel's responsibility to verify format of the messages
> it reads. Moved the check to OnMessage() implementations.
>
> Please review this at https://codereview.chromium.org/615543004/
>
> SVN Base: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+12, -6 lines):
>   M remoting/host/it2me/it2me_native_messaging_host.cc
>   M remoting/host/native_messaging/pipe_messaging_channel.cc
>   M remoting/host/setup/me2me_native_messaging_host.cc
>
>
> Index: remoting/host/it2me/it2me_native_messaging_host.cc
> diff --git a/remoting/host/it2me/it2me_native_messaging_host.cc
> b/remoting/host/it2me/it2me_native_messaging_host.cc
> index 6fb65df5ec7548a40489fdd78c90cec94bd21009..
> 35d578a7aa2abfde251f31ef9f3ecf42beee01ef 100644
> --- a/remoting/host/it2me/it2me_native_messaging_host.cc
> +++ b/remoting/host/it2me/it2me_native_messaging_host.cc
> @@ -87,6 +87,12 @@ void It2MeNativeMessagingHost::Start(const
> base::Closure& quit_closure) {
>  void It2MeNativeMessagingHost::OnMessage(scoped_ptr<base::Value>
> message) {
>    DCHECK(task_runner()->BelongsToCurrentThread());
>
> +  if (message->GetType() != base::Value::TYPE_DICTIONARY) {
> +    LOG(ERROR) << "Received a message that's not a dictionary.";
> +    channel_->SendMessage(nullptr);
> +    return;
> +  }
> +
>    scoped_ptr<base::DictionaryValue> message_dict(
>        static_cast<base::DictionaryValue*>(message.release()));
>    scoped_ptr<base::DictionaryValue> response(new
> base::DictionaryValue());
> Index: remoting/host/native_messaging/pipe_messaging_channel.cc
> diff --git a/remoting/host/native_messaging/pipe_messaging_channel.cc
> b/remoting/host/native_messaging/pipe_messaging_channel.cc
> index 0f3fe110ba3d481657f61c3bc1f2b015ba42f83e..
> 399bfa8aa9ae1fe795e70703309d747473ed0c63 100644
> --- a/remoting/host/native_messaging/pipe_messaging_channel.cc
> +++ b/remoting/host/native_messaging/pipe_messaging_channel.cc
> @@ -72,12 +72,6 @@ void PipeMessagingChannel::Start(EventHandler*
> event_handler) {
>  void PipeMessagingChannel::ProcessMessage(scoped_ptr<base::Value>
> message) {
>    DCHECK(CalledOnValidThread());
>
> -  if (message->GetType() != base::Value::TYPE_DICTIONARY) {
> -    LOG(ERROR) << "Expected DictionaryValue";
> -    Shutdown();
> -    return;
> -  }
> -
>    if (event_handler_)
>      event_handler_->OnMessage(message.Pass());
>  }
> Index: remoting/host/setup/me2me_native_messaging_host.cc
> diff --git a/remoting/host/setup/me2me_native_messaging_host.cc
> b/remoting/host/setup/me2me_native_messaging_host.cc
> index fc77b9f963e180f404eee4f811aa5dc862c9317f..
> 574a37e3e6ead351f18156eb418cac914be8a0f4 100644
> --- a/remoting/host/setup/me2me_native_messaging_host.cc
> +++ b/remoting/host/setup/me2me_native_messaging_host.cc
> @@ -104,6 +104,12 @@ void Me2MeNativeMessagingHost::Start(
>  void Me2MeNativeMessagingHost::OnMessage(scoped_ptr<base::Value>
> message) {
>    DCHECK(thread_checker_.CalledOnValidThread());
>
> +  if (message->GetType() != base::Value::TYPE_DICTIONARY) {
> +    LOG(ERROR) << "Received a message that's not a dictionary.";
> +    channel_->SendMessage(nullptr);
> +    return;
> +  }
> +
>    scoped_ptr<base::DictionaryValue> message_dict(
>        static_cast<base::DictionaryValue*>(message.release()));
>    scoped_ptr<base::DictionaryValue> response(new
> base::DictionaryValue());
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Sergey Ulanov, 2014-09-29 19:43:38]

On Mon, Sep 29, 2014 at 12:29 PM, Wez <wez@google.com> wrote:

> Why not use GetAsDictionary, to keep things type-safe?
>
Using GetAsDictionary() to convert base::Vector to base::DictionaryValue
makes the code awkward when the result needs to be stored in scoped_ptr<>,
because you need to call scoped_ptr<>.release() and ignore the result.
static_cast<> is ugly, but it's not more ugly than using GetAsDictionary()
*.*



>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Wez, 2014-09-29 20:14:37]

Fair point.

In that case, at least use IsType(). :)
On 29 Sep 2014 20:43, "Sergey Ulanov" <sergeyu@chromium.org> wrote:

> On Mon, Sep 29, 2014 at 12:29 PM, Wez <wez@google.com> wrote:
>
>> Why not use GetAsDictionary, to keep things type-safe?
>>
> Using GetAsDictionary() to convert base::Vector to base::DictionaryValue
> makes the code awkward when the result needs to be stored in scoped_ptr<>,
> because you need to call scoped_ptr<>.release() and ignore the result.
> static_cast<> is ugly, but it's not more ugly than using GetAsDictionary()
> *.*
>
>
>
>>
>>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kelvinp, 2014-09-29 20:20:48]

lgtm

[Sergey Ulanov, 2014-09-29 20:34:28]

On 2014/09/29 20:14:37, Wez wrote:
> Fair point.
> 
> In that case, at least use IsType(). :)

I'll fix it here and in all other places in remoting in a separate CL.

[Sergey Ulanov, 2014-09-29 20:34:36]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2014-09-29 20:35:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/615543004/1

[commit-bot: I haz the power, 2014-09-29 21:30:28]

Committed patchset #1 (id:1) as 1086b167b42d2c2b751eaa8ea58d4f69c194f999

[commit-bot: I haz the power, 2014-09-29 21:33:21]

Patchset 1 (id:??) landed as
https://crrev.com/c827ad1c743fa8eb5d33f609f6a1e1a317af7c1a
Cr-Commit-Position: refs/heads/master@{#297274}