[Chromecast] Fix PacketHeader.sample_rate overflow

We used to assume 16bits is sufficient, but somehow it's not always the
case. In this cl, let handshake message and pcm audio message no longer
share the same packet header. I.e., let them have their own header
struct and read/make methods.

Besides, add more unittest to cover packet header poluate/parse.

Merge-With: eureka-internal/453374
Bug: internal: 168457620
Test: Unittest.
Change-Id: I6e10fe9d58974646029ba222229e2f6d4df8acd5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2413110
Commit-Queue: Kehuang Li <kehuangli@chromium.org>
Reviewed-by: Kenneth MacKay <kmackay@chromium.org>
Reviewed-by: Yuchen Liu <yucliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809715}

chromecast/media/audio/BUILD.gn

@@ -108,6 +108,7 @@ cast_source_set("audio") {
     "//chromecast/base",
     "//chromecast/common/mojom",
     "//chromecast/media/api",
+    "//chromecast/media/audio/capture_service:common",
     "//chromecast/media/audio/capture_service:receiver",
     "//chromecast/media/audio/capture_service:utils",
     "//chromecast/media/audio/mixer_service:common",

chromecast/media/audio/capture_service/capture_service_receiver.cc

@@ -91,10 +91,8 @@ CaptureServiceReceiver::Socket::~Socket() = default;
 
 bool CaptureServiceReceiver::Socket::SendRequest() {
   DCHECK_EQ(state_, State::kInit);
-  auto request_buffer = capture_service::MakeMessage(
-      capture_service::PacketInfo{capture_service::MessageType::kHandshake,
-                                  request_stream_info_, 0 /* timestamp_us */},
-      nullptr /* data */, 0 /* data_size */);
+  auto request_buffer =
+      capture_service::MakeHandshakeMessage(request_stream_info_);
   if (!request_buffer) {
     return false;
   }
@@ -157,9 +155,9 @@ bool CaptureServiceReceiver::Socket::OnMessage(char* data, size_t size) {
 
 bool CaptureServiceReceiver::Socket::HandleAck(char* data, size_t size) {
   DCHECK_EQ(state_, State::kWaitForAck);
-  capture_service::PacketInfo info;
-  if (!capture_service::ReadHeader(data, size, &info) ||
-      !delegate_->OnInitialStreamInfo(info.stream_info)) {
+  capture_service::StreamInfo info;
+  if (!capture_service::ReadHandshakeMessage(data, size, &info) ||
+      !delegate_->OnInitialStreamInfo(info)) {
     ReportErrorAndStop();
     return false;
   }

chromecast/media/audio/capture_service/capture_service_receiver_unittest.cc

@@ -34,30 +34,18 @@ constexpr StreamInfo kStreamInfo =
                SampleFormat::PLANAR_FLOAT,
                16000,
                160};
-constexpr PacketHeader kHandshakePacketHeader =
-    PacketHeader{0,
+constexpr HandshakePacket kHandshakePacket =
+    HandshakePacket{0,
                     static_cast<uint8_t>(MessageType::kHandshake),
                     static_cast<uint8_t>(kStreamInfo.stream_type),
                     static_cast<uint8_t>(kStreamInfo.audio_codec),
-                 kStreamInfo.num_channels,
-                 kStreamInfo.sample_rate,
-                 kStreamInfo.frames_per_buffer};
-constexpr PacketHeader kPcmAudioPacketHeader =
-    PacketHeader{0,
-                 static_cast<uint8_t>(MessageType::kPcmAudio),
-                 static_cast<uint8_t>(kStreamInfo.stream_type),
                     static_cast<uint8_t>(kStreamInfo.sample_format),
                     kStreamInfo.num_channels,
-                 kStreamInfo.sample_rate,
-                 0};
-
-void FillHeader(char* buf, uint16_t size, const PacketHeader& header) {
-  base::WriteBigEndian(buf, size);
-  memcpy(buf + sizeof(size),
-         reinterpret_cast<const char*>(&header) +
-             offsetof(struct PacketHeader, message_type),
-         sizeof(header) - offsetof(struct PacketHeader, message_type));
-}
+                    kStreamInfo.frames_per_buffer,
+                    kStreamInfo.sample_rate};
+constexpr PcmPacketHeader kPcmAudioPacketHeader =
+    PcmPacketHeader{0, static_cast<uint8_t>(MessageType::kPcmAudio),
+                    static_cast<uint8_t>(kStreamInfo.stream_type), 0};
 
 class MockStreamSocket : public chromecast::MockStreamSocket {
  public:
@@ -95,7 +83,7 @@ TEST_F(CaptureServiceReceiverTest, StartStop) {
   auto socket1 = std::make_unique<MockStreamSocket>();
   auto socket2 = std::make_unique<MockStreamSocket>();
   EXPECT_CALL(*socket1, Connect).WillOnce(Return(net::OK));
-  EXPECT_CALL(*socket1, Write).WillOnce(Return(16));
+  EXPECT_CALL(*socket1, Write).WillOnce(Return(sizeof(HandshakePacket)));
   EXPECT_CALL(*socket1, Read).WillOnce(Return(net::ERR_IO_PENDING));
   EXPECT_CALL(*socket2, Connect).WillOnce(Return(net::OK));
 
@@ -135,17 +123,19 @@ TEST_F(CaptureServiceReceiverTest, SendRequest) {
       .WillOnce(Invoke([](net::IOBuffer* buf, int buf_len,
                           net::CompletionOnceCallback,
                           const net::NetworkTrafficAnnotationTag&) {
-        EXPECT_EQ(buf_len, static_cast<int>(sizeof(PacketHeader)));
+        EXPECT_EQ(buf_len, static_cast<int>(sizeof(HandshakePacket)));
         const char* data = buf->data();
         uint16_t size;
         base::ReadBigEndian(data, &size);
-        EXPECT_EQ(size, sizeof(PacketHeader) - sizeof(size));
-        PacketHeader header;
-        std::memcpy(&header, data, sizeof(PacketHeader));
-        EXPECT_EQ(header.message_type, kHandshakePacketHeader.message_type);
-        EXPECT_EQ(header.stream_type, kHandshakePacketHeader.stream_type);
-        EXPECT_EQ(header.codec_or_sample_format,
-                  kHandshakePacketHeader.codec_or_sample_format);
+        EXPECT_EQ(size, sizeof(HandshakePacket) - sizeof(size));
+        HandshakePacket packet;
+        std::memcpy(&packet, data, sizeof(HandshakePacket));
+        EXPECT_EQ(packet.message_type, kHandshakePacket.message_type);
+        EXPECT_EQ(packet.stream_type, kHandshakePacket.stream_type);
+        EXPECT_EQ(packet.audio_codec, kHandshakePacket.audio_codec);
+        EXPECT_EQ(packet.num_channels, kHandshakePacket.num_channels);
+        EXPECT_EQ(packet.num_frames, kHandshakePacket.num_frames);
+        EXPECT_EQ(packet.sample_rate, kHandshakePacket.sample_rate);
         return buf_len;
       }));
   EXPECT_CALL(*socket, Read).WillOnce(Return(net::ERR_IO_PENDING));
@@ -161,26 +151,24 @@ TEST_F(CaptureServiceReceiverTest, SendRequest) {
 TEST_F(CaptureServiceReceiverTest, ReceivePcmAudioMessage) {
   auto socket = std::make_unique<MockStreamSocket>();
   EXPECT_CALL(*socket, Connect).WillOnce(Return(net::OK));
-  EXPECT_CALL(*socket, Write).WillOnce(Return(16));
+  EXPECT_CALL(*socket, Write).WillOnce(Return(sizeof(HandshakePacket)));
   EXPECT_CALL(*socket, Read)
       // Ack message.
       .WillOnce(Invoke(
           [](net::IOBuffer* buf, int buf_len, net::CompletionOnceCallback) {
-            int total_size = sizeof(PacketHeader);
+            int total_size = sizeof(HandshakePacket);
             EXPECT_GE(buf_len, total_size);
-            uint16_t size = total_size - sizeof(uint16_t);
-            PacketHeader header = kHandshakePacketHeader;
-            FillHeader(buf->data(), size, header);
+            FillBuffer(buf->data(), total_size, &kHandshakePacket.message_type,
+                       sizeof(HandshakePacket) - sizeof(uint16_t));
             return total_size;
           }))
       // Audio message.
       .WillOnce(Invoke([](net::IOBuffer* buf, int buf_len,
                           net::CompletionOnceCallback) {
-        int total_size = sizeof(PacketHeader) + DataSizeInBytes(kStreamInfo);
+        int total_size = sizeof(PcmPacketHeader) + DataSizeInBytes(kStreamInfo);
         EXPECT_GE(buf_len, total_size);
-        uint16_t size = total_size - sizeof(uint16_t);
-        PacketHeader header = kPcmAudioPacketHeader;
-        FillHeader(buf->data(), size, header);
+        FillBuffer(buf->data(), total_size, &kPcmAudioPacketHeader.message_type,
+                   sizeof(PcmPacketHeader) - sizeof(uint16_t));
         return total_size;  // No need to fill audio frames.
       }))
       .WillOnce(Return(net::ERR_IO_PENDING));
@@ -198,7 +186,7 @@ TEST_F(CaptureServiceReceiverTest, ReceivePcmAudioMessage) {
 TEST_F(CaptureServiceReceiverTest, ReceiveMetadataMessage) {
   auto socket = std::make_unique<MockStreamSocket>();
   EXPECT_CALL(*socket, Connect).WillOnce(Return(net::OK));
-  EXPECT_CALL(*socket, Write).WillOnce(Return(16));
+  EXPECT_CALL(*socket, Write).WillOnce(Return(sizeof(HandshakePacket)));
   EXPECT_CALL(*socket, Read)
       .WillOnce(Invoke(
           [](net::IOBuffer* buf, int buf_len, net::CompletionOnceCallback) {
@@ -223,7 +211,7 @@ TEST_F(CaptureServiceReceiverTest, ReceiveMetadataMessage) {
 TEST_F(CaptureServiceReceiverTest, ReceiveError) {
   auto socket = std::make_unique<MockStreamSocket>();
   EXPECT_CALL(*socket, Connect).WillOnce(Return(net::OK));
-  EXPECT_CALL(*socket, Write).WillOnce(Return(16));
+  EXPECT_CALL(*socket, Write).WillOnce(Return(sizeof(HandshakePacket)));
   EXPECT_CALL(*socket, Read).WillOnce(Return(net::ERR_CONNECTION_RESET));
   EXPECT_CALL(delegate_, OnCaptureError);
 
@@ -234,7 +222,7 @@ TEST_F(CaptureServiceReceiverTest, ReceiveError) {
 TEST_F(CaptureServiceReceiverTest, ReceiveEosMessage) {
   auto socket = std::make_unique<MockStreamSocket>();
   EXPECT_CALL(*socket, Connect).WillOnce(Return(net::OK));
-  EXPECT_CALL(*socket, Write).WillOnce(Return(16));
+  EXPECT_CALL(*socket, Write).WillOnce(Return(sizeof(HandshakePacket)));
   EXPECT_CALL(*socket, Read).WillOnce(Return(0));
   EXPECT_CALL(delegate_, OnCaptureError);
 

chromecast/media/audio/capture_service/constants.h

@@ -78,20 +78,8 @@ struct StreamInfo {
   int frames_per_buffer = 0;
 };
 
-// Info describes the message packet. PacketInfo is only for message types that
-// support packet header, i.e., kHandshake and kPcmAudio. |timestamp_us| is
-// about when the buffer is captured. If the audio source is from ALSA, i.e.,
-// stream type is raw mic, it's the ALSA capture timestamp; otherwise, it may be
-// shifted based on the samples and sample rate upon raw mic input.
-struct PacketInfo {
-  MessageType message_type;
-  StreamInfo stream_info;
-  int64_t timestamp_us = 0;
-};
-
-// Size of a message header. The header can be parsed into PacketInfo with
-// methods in message_parsing_utils.h
-constexpr size_t kMessageHeaderBytes = 14;
+// Size of a PCM audio message header.
+constexpr size_t kPcmAudioHeaderBytes = 10;
 
 }  // namespace capture_service
 }  // namespace media

chromecast/media/audio/capture_service/message_parsing_fuzzer.cc

@@ -14,8 +14,8 @@ struct Environment {
 
 extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
   static Environment env;
-  PacketInfo info;
-  chromecast::media::capture_service::ReadHeader(
+  StreamInfo info;
+  chromecast::media::capture_service::ReadHandshakeMessage(
       reinterpret_cast<const char*>(data), size, &info);
   return 0;
 }

chromecast/media/audio/capture_service/message_parsing_utils.h

@@ -16,32 +16,34 @@ namespace chromecast {
 namespace media {
 namespace capture_service {
 
-// Read message header to |packet_info|, and return whether success.
-// The header of the message consists of <uint8_t message_type>
-// <uint8_t stream_type> <uint8_t audio_codec|sample_format> <uint8_t channels>
-// <uint16_t sample_rate> <uint64_t frames_per_buffer|timestamp_us>.
-// If |message_type| is kHandshake, it is a handshake message that has
-// |audio_codec| and |frames_per_buffer|, otherwise if |message_type| is
-// kPcmAudio, it's a PCM audio data message that has |sample_format| and
-// |timestamp_us|. Note it cannot be used to read kOpusAudio or kMetadata
-// messages, which don't have header besides |message_type| bits. Note
-// |packet_info| will be untouched if fails to read header. Note unsigned
-// |timestamp_us| will be converted to signed |timestamp| if valid. Note |data|
-// here has been parsed firstly by SmallMessageSocket, and thus doesn't have
-// <uint16_t size> bits.
-bool ReadHeader(const char* data, size_t size, PacketInfo* packet_info);
-
-// Make a IO buffer for stream message. It will populate the header with
-// |packet_info|, and copy |data| into the message if packet has audio and
-// |data| is not null. The returned buffer will have a length of |data_size| +
-// header size. Return nullptr if fails. Caller must guarantee the memory of
-// |data| has at least |data_size| when has audio.
+// Read message header, check if it matches |stream_info|, retrieve timestamp,
+// and return whether success.
+// Note |data| here has been parsed firstly by SmallMessageSocket, and thus
+// doesn't have <uint16_t size> bits.
+bool ReadPcmAudioHeader(const char* data,
+                        size_t size,
+                        const StreamInfo& stream_info,
+                        int64_t* timestamp_us);
+
+// Make a IO buffer for stream message. It will populate the header and copy
+// |data| into the message if packet has audio and |data| is not null. The
+// returned buffer will have a length of |data_size| + header size. Return
+// nullptr if fails. Caller must guarantee the memory of |data| has at least
+// |data_size| when has audio.
 // Note buffer will be sent with SmallMessageSocket, and thus contains a uint16
 // size field in the very first.
-scoped_refptr<net::IOBufferWithSize> MakeMessage(const PacketInfo& packet_info,
+scoped_refptr<net::IOBufferWithSize> MakePcmAudioMessage(StreamType stream_type,
+                                                         int64_t timestamp_us,
                                                          const char* data,
                                                          size_t data_size);
 
+// Make a IO buffer for handshake message. It will populate the header with
+// |stream_info|. Return nullptr if fails.
+// Note buffer will be sent with SmallMessageSocket, and thus contains a uint16
+// size field in the very first.
+scoped_refptr<net::IOBufferWithSize> MakeHandshakeMessage(
+    const StreamInfo& stream_info);
+
 // Make a IO buffer for serialized message. It will populate message size and
 // type fields, and copy |data| into the message. The returned buffer will have
 // a length of |data_size| + sizeof(uint8_t message_type) + sizeof(uint16_t
@@ -60,21 +62,43 @@ bool ReadDataToAudioBus(const StreamInfo& stream_info,
                         size_t size,
                         ::media::AudioBus* audio_bus);
 
-// Read the header part of the PCM audio message to packet info and the audio
-// data part to audio bus, and return whether success. This will run
-// ReadHeader() and ReadDataToAudioBus() in the underlying implementation.
+// Read the PCM audio message and copy the audio data to audio bus, as well as
+// the timestamp. Return whether success. This will run ReadPcmAudioHeader() and
+// ReadDataToAudioBus() in the underlying implementation.
 bool ReadPcmAudioMessage(const char* data,
                          size_t size,
-                         PacketInfo* packet_info,
+                         const StreamInfo& stream_info,
+                         int64_t* timestamp_us,
                          ::media::AudioBus* audio_bus);
 
-// Populate header of the message, including the SmallMessageSocket size bits.
-// Note this is used by unittest, user should use MakeMessage directly.
-char* PopulateHeader(char* data, size_t size, const PacketInfo& stream_info);
+// Read the handshake message to |stream_info|, and return true on success.
+bool ReadHandshakeMessage(const char* data,
+                          size_t size,
+                          StreamInfo* stream_info);
 
 // Return the expected size of the data of a stream message with |stream_info|.
 size_t DataSizeInBytes(const StreamInfo& stream_info);
 
+// Following methods are exposed for unittests:
+
+// Write |buf_size|, in big-endian order, to |buf|, and fill |data| to |buf|
+// afterward.
+void FillBuffer(char* buf, size_t buf_size, const void* data, size_t data_size);
+
+// Populate header of the PCM audio message, including the SmallMessageSocket
+// size bits.
+// Note this is used by unittest, user should use MakePcmAudioMessage directly.
+char* PopulatePcmAudioHeader(char* data,
+                             size_t size,
+                             StreamType stream_type,
+                             int64_t timestamp_us);
+
+// Populate the handshake message, including the SmallMessageSocket size bits.
+// Note this is used by unittest, user should use MakeHandshakeMessage directly.
+void PopulateHandshakeMessage(char* data,
+                              size_t size,
+                              const StreamInfo& stream_info);
+
 }  // namespace capture_service
 }  // namespace media
 }  // namespace chromecast

chromecast/media/audio/capture_service/packet_header.h

@@ -11,19 +11,29 @@ namespace chromecast {
 namespace media {
 namespace capture_service {
 
-// Memory block of a packet header. Changes to it need to make sure about the
-// memory alignment to avoid extra paddings being inserted. It reflects real
-// packet header structure, however, the |size| bits are in big-endian order,
-// and thus is only for padding purpose in this struct, when all bytes after it
-// represent a message header.
-struct __attribute__((__packed__)) PacketHeader {
+// Memory block of a PCM audio packet header. Changes to it need to ensure the
+// size is a multiple of 4 bytes. It reflects real packet header structure,
+// however, the |size| bits are in big-endian order, and thus is only for
+// padding purpose in this struct, when all bytes after it represent a message
+// header.
+struct __attribute__((__packed__)) PcmPacketHeader {
   uint16_t size;
   uint8_t message_type;
   uint8_t stream_type;
-  uint8_t codec_or_sample_format;
+  int64_t timestamp_us;
+};
+
+// Memory block of a handshake packet. There is no size restriction for this
+// structure.
+struct __attribute__((__packed__)) HandshakePacket {
+  uint16_t size;
+  uint8_t message_type;
+  uint8_t stream_type;
+  uint8_t audio_codec;
+  uint8_t sample_format;
   uint8_t num_channels;
-  uint16_t sample_rate;
-  int64_t timestamp_or_frames;
+  uint16_t num_frames;
+  uint32_t sample_rate;
 };
 
 }  // namespace capture_service

chromecast/media/audio/cast_audio_input_stream.cc

@@ -48,14 +48,14 @@ bool CastAudioInputStream::Open() {
 
   audio_bus_ = ::media::AudioBus::Create(audio_params_.channels(),
                                          audio_params_.frames_per_buffer());
-  capture_service_receiver_ = std::make_unique<CaptureServiceReceiver>(
-      capture_service::StreamInfo{
+  stream_info_ = capture_service::StreamInfo{
       capture_service::StreamType::kSoftwareEchoCancelled,
       capture_service::AudioCodec::kPcm, audio_params_.channels(),
       // Format doesn't matter in the request.
-          capture_service::SampleFormat::LAST_FORMAT,
-          audio_params_.sample_rate(), audio_params_.frames_per_buffer()},
-      this);
+      capture_service::SampleFormat::LAST_FORMAT, audio_params_.sample_rate(),
+      audio_params_.frames_per_buffer()};
+  capture_service_receiver_ =
+      std::make_unique<CaptureServiceReceiver>(stream_info_, this);
   return true;
 }
 
@@ -117,33 +117,40 @@ void CastAudioInputStream::SetOutputDeviceForAec(
 bool CastAudioInputStream::OnInitialStreamInfo(
     const capture_service::StreamInfo& stream_info) {
   const bool is_params_match =
-      stream_info.stream_type ==
-          capture_service::StreamType::kSoftwareEchoCancelled &&
-      stream_info.audio_codec == capture_service::AudioCodec::kPcm &&
-      stream_info.num_channels == audio_params_.channels() &&
-      stream_info.sample_rate == audio_params_.sample_rate() &&
-      stream_info.frames_per_buffer == audio_params_.frames_per_buffer();
+      stream_info.stream_type == stream_info_.stream_type &&
+      stream_info.audio_codec == stream_info_.audio_codec &&
+      stream_info.num_channels == stream_info_.num_channels &&
+      stream_info.sample_rate == stream_info_.sample_rate &&
+      stream_info.frames_per_buffer == stream_info_.frames_per_buffer;
   LOG_IF(ERROR, !is_params_match)
-      << "Got different parameters from sender, sample_rate: "
-      << audio_params_.sample_rate() << " Hz -> " << stream_info.sample_rate
-      << " Hz, num_channels: " << audio_params_.channels() << " -> "
+      << "Got different parameters from sender, stream_type: "
+      << static_cast<int>(stream_info_.stream_type) << " -> "
+      << static_cast<int>(stream_info.stream_type)
+      << ", audio_codec: " << static_cast<int>(stream_info_.audio_codec)
+      << " -> " << static_cast<int>(stream_info.audio_codec)
+      << ", sample_rate: " << stream_info_.sample_rate << " Hz -> "
+      << stream_info.sample_rate
+      << " Hz, num_channels: " << stream_info_.num_channels << " -> "
       << stream_info.num_channels
-      << ", frames_per_buffer: " << audio_params_.frames_per_buffer() << " -> "
+      << ", frames_per_buffer: " << stream_info_.frames_per_buffer << " -> "
       << stream_info.frames_per_buffer << ".";
+  stream_info_.sample_format = stream_info.sample_format;
+  LOG(INFO) << "Set sample_format: "
+            << static_cast<int>(stream_info.sample_format);
   return is_params_match;
 }
 
 bool CastAudioInputStream::OnCaptureData(const char* data, size_t size) {
-  capture_service::PacketInfo info;
-  if (!capture_service::ReadPcmAudioMessage(data, size, &info,
-                                            audio_bus_.get())) {
+  int64_t timestamp_us;
+  if (!capture_service::ReadPcmAudioMessage(data, size, stream_info_,
+                                            &timestamp_us, audio_bus_.get())) {
     return false;
   }
 
   DCHECK(input_callback_);
   input_callback_->OnData(
       audio_bus_.get(),
-      base::TimeTicks() + base::TimeDelta::FromMicroseconds(info.timestamp_us),
+      base::TimeTicks() + base::TimeDelta::FromMicroseconds(timestamp_us),
       /* volume */ 1.0);
   return true;
 }

chromecast/media/audio/cast_audio_input_stream.h

@@ -10,6 +10,7 @@
 
 #include "base/threading/thread_checker.h"
 #include "chromecast/media/audio/capture_service/capture_service_receiver.h"
+#include "chromecast/media/audio/capture_service/constants.h"
 #include "media/audio/audio_io.h"
 #include "media/base/audio_bus.h"
 #include "media/base/audio_parameters.h"
@@ -58,6 +59,7 @@ class CastAudioInputStream : public ::media::AudioInputStream,
   // may be null, if |this| is not created by audio manager, e.g., in unit test.
   ::media::AudioManagerBase* const audio_manager_;
   const ::media::AudioParameters audio_params_;
+  capture_service::StreamInfo stream_info_;
   std::unique_ptr<CaptureServiceReceiver> capture_service_receiver_;
   AudioInputCallback* input_callback_ = nullptr;
   std::unique_ptr<::media::AudioBus> audio_bus_;