Use safer IPC serializations in media_param_traits.cc

Use safer IPC serializations in media_param_traits.cc

The custom serialization routines for these classes used base::Pickle's
serialization/deserialization routines directly rather than using the
type-deducing ReadParam/WriteParam helpers. These helpers have the bonus
of using any pre-existing ParamTraits defined for the type, so the
custom serialization doesn't have to remember to check that enum values
are in range, etc.

BUG=504205
Committed: https://crrev.com/3c49f49b2c6d873fb5b23abc52cde08de7909b88
Cr-Commit-Position: refs/heads/master@{#336861}

[dcheng, 2015-06-29 21:46:54]

dcheng@chromium.org changed reviewers:
+ inferno@chromium.org, xhwang@chromium.org

[dcheng, 2015-06-29 21:46:55]

+xhwang for overall review, +inferno to double-check the IPC security bits.

https://codereview.chromium.org/1211273005/diff/60001/media/audio/audio_param...
File media/audio/audio_parameters.cc (left):

https://codereview.chromium.org/1211273005/diff/60001/media/audio/audio_param...
media/audio/audio_parameters.cc:72: (format_ < AUDIO_LAST_FORMAT) &&
Removing these two lines causes two unit tests to fail.

I have ask: what is the value of those unit tests? You have to do clearly
invalid things to get it to fail (convert an out-of-range int to an enum value
by casting it). The only way this could happen normally is:
1) Someone checks in extremely broken code. AudioParameters::IsValid() is a weak
defense against this anyway.
2) Some serialization/deserialization routine forgets to sanity check this. But
the only place we should be serializing/deserializing this is via IPC messages?
So I don't see how this could happen either.

Given this, would it make sense to just remove the unit tests in question?

[inferno, 2015-06-29 23:32:02]

inferno@chromium.org changed reviewers:
+ tsepez@chromium.org
- inferno@chromium.org

[inferno, 2015-06-29 23:32:38]

Adding Tom for ipc security bits.

[xhwang, 2015-06-29 23:53:15]

lgtm

https://codereview.chromium.org/1211273005/diff/60001/media/audio/audio_param...
File media/audio/audio_parameters.cc (left):

https://codereview.chromium.org/1211273005/diff/60001/media/audio/audio_param...
media/audio/audio_parameters.cc:72: (format_ < AUDIO_LAST_FORMAT) &&
On 2015/06/29 21:46:55, dcheng wrote:
> Removing these two lines causes two unit tests to fail.
> 
> I have ask: what is the value of those unit tests? You have to do clearly
> invalid things to get it to fail (convert an out-of-range int to an enum value
> by casting it). The only way this could happen normally is:
> 1) Someone checks in extremely broken code. AudioParameters::IsValid() is a
weak
> defense against this anyway.
> 2) Some serialization/deserialization routine forgets to sanity check this.
But
> the only place we should be serializing/deserializing this is via IPC
messages?
> So I don't see how this could happen either.
> 
> Given this, would it make sense to just remove the unit tests in question?

I agree :)

[Tom Sepez, 2015-06-30 15:37:07]

lgtm

https://codereview.chromium.org/1211273005/diff/60001/content/common/media/me...
File content/common/media/media_param_traits.cc (right):

https://codereview.chromium.org/1211273005/diff/60001/content/common/media/me...
content/common/media/media_param_traits.cc:39: if (!ReadParam(m, iter, &format)
|| !ReadParam(m, iter, &channel_layout) ||
nit: I'd still prefer one per line.

https://codereview.chromium.org/1211273005/diff/60001/media/audio/audio_param...
File media/audio/audio_parameters.cc (left):

https://codereview.chromium.org/1211273005/diff/60001/media/audio/audio_param...
media/audio/audio_parameters.cc:72: (format_ < AUDIO_LAST_FORMAT) &&
On 2015/06/29 23:53:15, xhwang wrote:
> On 2015/06/29 21:46:55, dcheng wrote:
> > Removing these two lines causes two unit tests to fail.
> > 
> > I have ask: what is the value of those unit tests? You have to do clearly
> > invalid things to get it to fail (convert an out-of-range int to an enum
value
> > by casting it). The only way this could happen normally is:
> > 1) Someone checks in extremely broken code. AudioParameters::IsValid() is a
> weak
> > defense against this anyway.
> > 2) Some serialization/deserialization routine forgets to sanity check this.
> But
> > the only place we should be serializing/deserializing this is via IPC
> messages?
> > So I don't see how this could happen either.
> > 
> > Given this, would it make sense to just remove the unit tests in question?
> 
> I agree :)

Now that the enums are being checked by IPC itself, we avoid the situation that
a crafted message from an untrusted process (renderer) could reach a trusted
browser process.  IsValid() would have to guard against that in the past, but it
doesn't anymore.

[dcheng, 2015-06-30 18:11:59]

https://codereview.chromium.org/1211273005/diff/60001/content/common/media/me...
File content/common/media/media_param_traits.cc (right):

https://codereview.chromium.org/1211273005/diff/60001/content/common/media/me...
content/common/media/media_param_traits.cc:39: if (!ReadParam(m, iter, &format)
|| !ReadParam(m, iter, &channel_layout) ||
On 2015/06/30 at 15:37:07, Tom Sepez (present) wrote:
> nit: I'd still prefer one per line.

This is due to clang-format: if you feel strongly about this, I can revert, but
I'd prefer not to fight the tooling.

https://codereview.chromium.org/1211273005/diff/60001/media/audio/audio_param...
File media/audio/audio_parameters.cc (left):

https://codereview.chromium.org/1211273005/diff/60001/media/audio/audio_param...
media/audio/audio_parameters.cc:72: (format_ < AUDIO_LAST_FORMAT) &&
On 2015/06/30 at 15:37:07, Tom Sepez (present) wrote:
> On 2015/06/29 23:53:15, xhwang wrote:
> > On 2015/06/29 21:46:55, dcheng wrote:
> > > Removing these two lines causes two unit tests to fail.
> > > 
> > > I have ask: what is the value of those unit tests? You have to do clearly
> > > invalid things to get it to fail (convert an out-of-range int to an enum
value
> > > by casting it). The only way this could happen normally is:
> > > 1) Someone checks in extremely broken code. AudioParameters::IsValid() is
a
> > weak
> > > defense against this anyway.
> > > 2) Some serialization/deserialization routine forgets to sanity check
this.
> > But
> > > the only place we should be serializing/deserializing this is via IPC
> > messages?
> > > So I don't see how this could happen either.
> > > 
> > > Given this, would it make sense to just remove the unit tests in question?
> > 
> > I agree :)
> 
> Now that the enums are being checked by IPC itself, we avoid the situation
that a crafted message from an untrusted process (renderer) could reach a
trusted browser process.  IsValid() would have to guard against that in the
past, but it doesn't anymore.

I just wanted to make sure that media code wasn't using IsValid() in unexpected
ways before I removed the tests for "invalid foramt".

Done!

[Tom Sepez, 2015-06-30 19:43:22]

https://codereview.chromium.org/1211273005/diff/60001/content/common/media/me...
File content/common/media/media_param_traits.cc (right):

https://codereview.chromium.org/1211273005/diff/60001/content/common/media/me...
content/common/media/media_param_traits.cc:39: if (!ReadParam(m, iter, &format)
|| !ReadParam(m, iter, &channel_layout) ||
> I'd prefer not to fight 
No worries.  Let's leave it as is.

[dcheng, 2015-06-30 20:08:29]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2015-06-30 20:08:29]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1211273005/#ps80001
(title: "Clean up unit tests")

[commit-bot: I haz the power, 2015-06-30 20:09:13]

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

[commit-bot: I haz the power, 2015-06-30 20:14:50]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-06-30 20:16:34]

Patchset 5 (id:??) landed as
https://crrev.com/3c49f49b2c6d873fb5b23abc52cde08de7909b88
Cr-Commit-Position: refs/heads/master@{#336861}