Use safer IPC serializations in media_param_traits.cc

media/audio/audio_parameters.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/audio/audio_parameters.h"
 
 #include "base/logging.h"
 #include "media/base/limits.h"
 
 namespace media {
@@ skipping 50 matching lines @@
                             int bits_per_sample, int frames_per_buffer) {
   format_ = format;
   channel_layout_ = channel_layout;
   channels_ = channels;
   sample_rate_ = sample_rate;
   bits_per_sample_ = bits_per_sample;
   frames_per_buffer_ = frames_per_buffer;
 }
 
 bool AudioParameters::IsValid() const {
-  return (format_ >= AUDIO_PCM_LINEAR) &&
-         (format_ < AUDIO_LAST_FORMAT) &&

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

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?

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

I agree :)

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

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]

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!

-         (channels_ > 0) &&
-         (channels_ <= media::limits::kMaxChannels) &&
+  return (channels_ > 0) && (channels_ <= media::limits::kMaxChannels) &&
          (channel_layout_ > CHANNEL_LAYOUT_UNSUPPORTED) &&
-         (channel_layout_ <= CHANNEL_LAYOUT_MAX) &&
          (sample_rate_ >= media::limits::kMinSampleRate) &&
          (sample_rate_ <= media::limits::kMaxSampleRate) &&
          (bits_per_sample_ > 0) &&
          (bits_per_sample_ <= media::limits::kMaxBitsPerSample) &&
          (frames_per_buffer_ > 0) &&
          (frames_per_buffer_ <= media::limits::kMaxSamplesPerPacket) &&
          (channel_layout_ == CHANNEL_LAYOUT_DISCRETE ||
           channels_ == ChannelLayoutToChannelCount(channel_layout_));
 }
 
@@ skipping 30 matching lines @@
   return format_ == other.format() &&
          sample_rate_ == other.sample_rate() &&
          channel_layout_ == other.channel_layout() &&
          channels_ == other.channels() &&
          bits_per_sample_ == other.bits_per_sample() &&
          frames_per_buffer_ == other.frames_per_buffer() &&
          effects_ == other.effects();
 }
 
 }  // namespace media