diff options
author | dskiba <dskiba@google.com> | 2015-09-30 10:24:30 -0700 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2015-09-30 17:25:11 +0000 |
commit | 6f3790a15d0a5463e18dd7220e4f2028a6318389 (patch) | |
tree | ab582b04b1e108eed2c239a4081b73c6d90a759b /ipc | |
parent | 4af4344076559b103a5045693a26e1422b3e211a (diff) | |
download | chromium_src-6f3790a15d0a5463e18dd7220e4f2028a6318389.zip chromium_src-6f3790a15d0a5463e18dd7220e4f2028a6318389.tar.gz chromium_src-6f3790a15d0a5463e18dd7220e4f2028a6318389.tar.bz2 |
Resize IPC input buffer to fit the next message.
Sometimes we can get IPC message size from its header. In those cases
we resize IPC::ChannelReader' overflow buffer to fit the entire message
to avoid growing / reallocating it as we receive message's data.
BUG=529940
Review URL: https://codereview.chromium.org/1345353004
Cr-Commit-Position: refs/heads/master@{#351586}
Diffstat (limited to 'ipc')
-rw-r--r-- | ipc/ipc_channel_reader.cc | 31 | ||||
-rw-r--r-- | ipc/ipc_channel_reader.h | 5 | ||||
-rw-r--r-- | ipc/ipc_channel_reader_unittest.cc | 65 | ||||
-rw-r--r-- | ipc/ipc_message.cc | 41 | ||||
-rw-r--r-- | ipc/ipc_message.h | 10 | ||||
-rw-r--r-- | ipc/ipc_message_unittest.cc | 103 |
6 files changed, 235 insertions, 20 deletions
diff --git a/ipc/ipc_channel_reader.cc b/ipc/ipc_channel_reader.cc index 031417b..5ca0e46 100644 --- a/ipc/ipc_channel_reader.cc +++ b/ipc/ipc_channel_reader.cc @@ -78,17 +78,15 @@ bool ChannelReader::TranslateInputData(const char* input_data, p = input_data; end = input_data + input_data_len; } else { - if (input_overflow_buf_.size() + input_data_len > - Channel::kMaximumMessageSize) { - input_overflow_buf_.clear(); - LOG(ERROR) << "IPC message is too big"; + if (!CheckMessageSize(input_overflow_buf_.size() + input_data_len)) return false; - } input_overflow_buf_.append(input_data, input_data_len); p = input_overflow_buf_.data(); end = p + input_overflow_buf_.size(); } + size_t next_message_size = 0; + // Dispatch all complete messages in the data buffer. while (p < end) { Message::NextMessageInfo info; @@ -127,6 +125,9 @@ bool ChannelReader::TranslateInputData(const char* input_data, p = info.message_end; } else { // Last message is partial. + next_message_size = info.message_size; + if (!CheckMessageSize(next_message_size)) + return false; break; } } @@ -134,6 +135,17 @@ bool ChannelReader::TranslateInputData(const char* input_data, // Save any partial data in the overflow buffer. input_overflow_buf_.assign(p, end - p); + if (!input_overflow_buf_.empty()) { + // We have something in the overflow buffer, which means that we will + // append the next data chunk (instead of parsing it directly). So we + // resize the buffer to fit the next message, to avoid repeatedly + // growing the buffer as we receive all message' data chunks. + next_message_size += Channel::kReadBufferSize - 1; + if (next_message_size > input_overflow_buf_.capacity()) { + input_overflow_buf_.reserve(next_message_size); + } + } + if (input_overflow_buf_.empty() && !DidEmptyInputBuffers()) return false; return true; @@ -249,5 +261,14 @@ void ChannelReader::StopObservingAttachmentBroker() { #endif // USE_ATTACHMENT_BROKER } +bool ChannelReader::CheckMessageSize(size_t size) { + if (size <= Channel::kMaximumMessageSize) { + return true; + } + input_overflow_buf_.clear(); + LOG(ERROR) << "IPC message is too big: " << size; + return false; +} + } // namespace internal } // namespace IPC diff --git a/ipc/ipc_channel_reader.h b/ipc/ipc_channel_reader.h index 3f3fc4579..4677522 100644 --- a/ipc/ipc_channel_reader.h +++ b/ipc/ipc_channel_reader.h @@ -127,6 +127,8 @@ class IPC_EXPORT ChannelReader : public SupportsAttachmentBrokering, private: FRIEND_TEST_ALL_PREFIXES(ChannelReaderTest, AttachmentAlreadyBrokered); FRIEND_TEST_ALL_PREFIXES(ChannelReaderTest, AttachmentNotYetBrokered); + FRIEND_TEST_ALL_PREFIXES(ChannelReaderTest, ResizeOverflowBuffer); + FRIEND_TEST_ALL_PREFIXES(ChannelReaderTest, InvalidMessageSize); typedef std::set<BrokerableAttachment::AttachmentId> AttachmentIdSet; @@ -156,6 +158,9 @@ class IPC_EXPORT ChannelReader : public SupportsAttachmentBrokering, void StartObservingAttachmentBroker(); void StopObservingAttachmentBroker(); + // Checks that |size| is a valid message size. Has side effects if it's not. + bool CheckMessageSize(size_t size); + Listener* listener_; // We read from the pipe into this buffer. Managed by DispatchInputData, do diff --git a/ipc/ipc_channel_reader_unittest.cc b/ipc/ipc_channel_reader_unittest.cc index 0058980..7728a81 100644 --- a/ipc/ipc_channel_reader_unittest.cc +++ b/ipc/ipc_channel_reader_unittest.cc @@ -4,6 +4,7 @@ #include "build/build_config.h" +#include <limits> #include <set> #include "ipc/attachment_broker.h" @@ -12,12 +13,13 @@ #include "ipc/placeholder_brokerable_attachment.h" #include "testing/gtest/include/gtest/gtest.h" -#if USE_ATTACHMENT_BROKER namespace IPC { namespace internal { namespace { +#if USE_ATTACHMENT_BROKER + class MockAttachment : public BrokerableAttachment { public: MockAttachment() {} @@ -53,6 +55,8 @@ class MockAttachmentBroker : public AttachmentBroker { } }; +#endif // USE_ATTACHMENT_BROKER + class MockChannelReader : public ChannelReader { public: MockChannelReader() @@ -92,8 +96,16 @@ class MockChannelReader : public ChannelReader { AttachmentBroker* broker_; }; +class ExposedMessage: public Message { + public: + using Message::Header; + using Message::header; +}; + } // namespace +#if USE_ATTACHMENT_BROKER + TEST(ChannelReaderTest, AttachmentAlreadyBrokered) { MockAttachmentBroker broker; MockChannelReader reader; @@ -129,6 +141,55 @@ TEST(ChannelReaderTest, AttachmentNotYetBrokered) { EXPECT_EQ(m, reader.get_last_dispatched_message()); } +#endif // USE_ATTACHMENT_BROKER + +#if !USE_ATTACHMENT_BROKER + +// We can determine message size from its header (and hence resize the buffer) +// only when attachment broker is not used, see IPC::Message::FindNext(). + +TEST(ChannelReaderTest, ResizeOverflowBuffer) { + MockChannelReader reader; + + ExposedMessage::Header header = {}; + + header.payload_size = 128 * 1024; + EXPECT_LT(reader.input_overflow_buf_.capacity(), header.payload_size); + EXPECT_TRUE(reader.TranslateInputData( + reinterpret_cast<const char*>(&header), sizeof(header))); + + // Once message header is available we resize overflow buffer to + // fit the entire message. + EXPECT_GE(reader.input_overflow_buf_.capacity(), header.payload_size); +} + +TEST(ChannelReaderTest, InvalidMessageSize) { + MockChannelReader reader; + + ExposedMessage::Header header = {}; + + size_t capacity_before = reader.input_overflow_buf_.capacity(); + + // Message is slightly larger than maximum allowed size + header.payload_size = Channel::kMaximumMessageSize + 1; + EXPECT_FALSE(reader.TranslateInputData( + reinterpret_cast<const char*>(&header), sizeof(header))); + EXPECT_LE(reader.input_overflow_buf_.capacity(), capacity_before); + + // Payload size is negative, overflow is detected by Pickle::PeekNext() + header.payload_size = static_cast<uint32_t>(-1); + EXPECT_FALSE(reader.TranslateInputData( + reinterpret_cast<const char*>(&header), sizeof(header))); + EXPECT_LE(reader.input_overflow_buf_.capacity(), capacity_before); + + // Payload size is maximum int32 value + header.payload_size = std::numeric_limits<int32_t>::max(); + EXPECT_FALSE(reader.TranslateInputData( + reinterpret_cast<const char*>(&header), sizeof(header))); + EXPECT_LE(reader.input_overflow_buf_.capacity(), capacity_before); +} + +#endif // !USE_ATTACHMENT_BROKER + } // namespace internal } // namespace IPC -#endif // USE_ATTACHMENT_BROKER diff --git a/ipc/ipc_message.cc b/ipc/ipc_message.cc index 02969ca..df28464 100644 --- a/ipc/ipc_message.cc +++ b/ipc/ipc_message.cc @@ -137,7 +137,8 @@ void Message::set_received_time(int64_t time) const { #endif Message::NextMessageInfo::NextMessageInfo() - : message_found(false), pickle_end(nullptr), message_end(nullptr) {} + : message_size(0), message_found(false), pickle_end(nullptr), + message_end(nullptr) {} Message::NextMessageInfo::~NextMessageInfo() {} Message::SerializedAttachmentIds @@ -166,17 +167,28 @@ void Message::FindNext(const char* range_start, const char* range_end, NextMessageInfo* info) { DCHECK(info); - const char* pickle_end = - base::Pickle::FindNext(sizeof(Header), range_start, range_end); - if (!pickle_end) + info->message_found = false; + info->message_size = 0; + + size_t pickle_size = 0; + if (!base::Pickle::PeekNext(sizeof(Header), + range_start, range_end, &pickle_size)) return; - info->pickle_end = pickle_end; + + bool have_entire_pickle = + static_cast<size_t>(range_end - range_start) >= pickle_size; #if USE_ATTACHMENT_BROKER + // TODO(dskiba): determine message_size when entire pickle is not available + + if (!have_entire_pickle) + return; + + const char* pickle_end = range_start + pickle_size; + // The data is not copied. - size_t pickle_len = static_cast<size_t>(pickle_end - range_start); - Message message(range_start, static_cast<int>(pickle_len)); - int num_attachments = message.header()->num_brokered_attachments; + Message message(range_start, static_cast<int>(pickle_size)); + size_t num_attachments = message.header()->num_brokered_attachments; // Check for possible overflows. size_t max_size_t = std::numeric_limits<size_t>::max(); @@ -184,12 +196,12 @@ void Message::FindNext(const char* range_start, return; size_t attachment_length = num_attachments * BrokerableAttachment::kNonceSize; - if (pickle_len > max_size_t - attachment_length) + if (pickle_size > max_size_t - attachment_length) return; // Check whether the range includes the attachments. size_t buffer_length = static_cast<size_t>(range_end - range_start); - if (buffer_length < attachment_length + pickle_len) + if (buffer_length < attachment_length + pickle_size) return; for (int i = 0; i < num_attachments; ++i) { @@ -201,10 +213,19 @@ void Message::FindNext(const char* range_start, } info->message_end = pickle_end + num_attachments * BrokerableAttachment::kNonceSize; + info->message_size = info->message_end - range_start; #else + info->message_size = pickle_size; + + if (!have_entire_pickle) + return; + + const char* pickle_end = range_start + pickle_size; + info->message_end = pickle_end; #endif // USE_ATTACHMENT_BROKER + info->pickle_end = pickle_end; info->message_found = true; } diff --git a/ipc/ipc_message.h b/ipc/ipc_message.h index 1a8a18f..22d1c99 100644 --- a/ipc/ipc_message.h +++ b/ipc/ipc_message.h @@ -9,6 +9,7 @@ #include <string> +#include "base/gtest_prod_util.h" #include "base/memory/ref_counted.h" #include "base/pickle.h" #include "base/trace_event/trace_event.h" @@ -168,10 +169,14 @@ class IPC_EXPORT Message : public base::Pickle { // The static method FindNext() returns several pieces of information, which // are aggregated into an instance of this struct. - struct NextMessageInfo { + struct IPC_EXPORT NextMessageInfo { NextMessageInfo(); ~NextMessageInfo(); + // Total message size. Always valid if |message_found| is true. + // If |message_found| is false but we could determine message size + // from the header, this field is non-zero. Otherwise it's zero. + size_t message_size; // Whether an entire message was found in the given memory range. bool message_found; // Only filled in if |message_found| is true. @@ -313,6 +318,9 @@ class IPC_EXPORT Message : public base::Pickle { mutable LogData* log_data_; mutable bool dont_log_; #endif + + FRIEND_TEST_ALL_PREFIXES(IPCMessageTest, FindNext); + FRIEND_TEST_ALL_PREFIXES(IPCMessageTest, FindNextOverflow); }; //------------------------------------------------------------------------------ diff --git a/ipc/ipc_message_unittest.cc b/ipc/ipc_message_unittest.cc index aea7054..a4f9af0 100644 --- a/ipc/ipc_message_unittest.cc +++ b/ipc/ipc_message_unittest.cc @@ -6,6 +6,8 @@ #include <string.h> +#include <limits> + #include "base/memory/scoped_ptr.h" #include "base/strings/utf_string_conversions.h" #include "base/values.h" @@ -25,7 +27,7 @@ IPC_MESSAGE_CONTROL1(TestMsgClassI, int) IPC_SYNC_MESSAGE_CONTROL1_1(TestMsgClassIS, int, std::string) -namespace { +namespace IPC { TEST(IPCMessageTest, BasicMessageTest) { int v1 = 10; @@ -115,6 +117,101 @@ TEST(IPCMessageTest, DictionaryValue) { EXPECT_FALSE(IPC::ReadParam(&bad_msg, &iter, &output)); } +TEST(IPCMessageTest, FindNext) { + IPC::Message message; + EXPECT_TRUE(message.WriteString("Goooooooogle")); + EXPECT_TRUE(message.WriteInt(111)); + + std::vector<char> message_data(message.size() + 7); + memcpy(message_data.data(), message.data(), message.size()); + + const char* data_start = message_data.data(); + const char* data_end = data_start + message.size(); + + IPC::Message::NextMessageInfo next; + + // Data range contains the entire message plus some extra bytes + IPC::Message::FindNext(data_start, data_end + 1, &next); + EXPECT_TRUE(next.message_found); + EXPECT_EQ(next.message_size, message.size()); + EXPECT_EQ(next.pickle_end, data_end); + EXPECT_EQ(next.message_end, data_end); + + // Data range exactly contains the entire message + IPC::Message::FindNext(data_start, data_end, &next); + EXPECT_TRUE(next.message_found); + EXPECT_EQ(next.message_size, message.size()); + EXPECT_EQ(next.pickle_end, data_end); + EXPECT_EQ(next.message_end, data_end); + + // Data range doesn't contain the entire message + // (but contains the message header) + IPC::Message::FindNext(data_start, data_end - 1, &next); + EXPECT_FALSE(next.message_found); +#if USE_ATTACHMENT_BROKER + EXPECT_EQ(next.message_size, 0u); +#else + EXPECT_EQ(next.message_size, message.size()); +#endif + + // Data range doesn't contain the message header + // (but contains the pickle header) + IPC::Message::FindNext(data_start, + data_start + sizeof(IPC::Message::Header) - 1, + &next); + EXPECT_FALSE(next.message_found); + EXPECT_EQ(next.message_size, 0u); + + // Data range doesn't contain the pickle header + IPC::Message::FindNext(data_start, + data_start + sizeof(base::Pickle::Header) - 1, + &next); + EXPECT_FALSE(next.message_found); + EXPECT_EQ(next.message_size, 0u); +} + +TEST(IPCMessageTest, FindNextOverflow) { + IPC::Message message; + EXPECT_TRUE(message.WriteString("Data")); + EXPECT_TRUE(message.WriteInt(777)); + + const char* data_start = reinterpret_cast<const char*>(message.data()); + const char* data_end = data_start + message.size(); + + IPC::Message::NextMessageInfo next; + + // Payload size is negative (defeats 'start + size > end' check) + message.header()->payload_size = static_cast<uint32_t>(-1); + IPC::Message::FindNext(data_start, data_end, &next); + EXPECT_FALSE(next.message_found); +#if USE_ATTACHMENT_BROKER + EXPECT_EQ(next.message_size, 0u); +#else + if (sizeof(size_t) > sizeof(uint32_t)) { + // No overflow, just insane message size + EXPECT_EQ(next.message_size, + message.header()->payload_size + sizeof(IPC::Message::Header)); + } else { + // Actual overflow, reported as max size_t + EXPECT_EQ(next.message_size, std::numeric_limits<size_t>::max()); + } +#endif + + // Payload size is max positive integer (defeats size < 0 check, while + // still potentially causing overflow down the road). + message.header()->payload_size = std::numeric_limits<int32_t>::max(); + IPC::Message::FindNext(data_start, data_end, &next); + EXPECT_FALSE(next.message_found); +#if USE_ATTACHMENT_BROKER + EXPECT_EQ(next.message_size, 0u); +#else + EXPECT_EQ(next.message_size, + message.header()->payload_size + sizeof(IPC::Message::Header)); +#endif +} + +namespace { + class IPCMessageParameterTest : public testing::Test { public: IPCMessageParameterTest() : extra_param_("extra_param"), called_(false) {} @@ -160,6 +257,8 @@ class IPCMessageParameterTest : public testing::Test { bool called_; }; +} // namespace + TEST_F(IPCMessageParameterTest, EmptyDispatcherWithParam) { TestMsgClassEmpty message; EXPECT_TRUE(OnMessageReceived(message)); @@ -186,4 +285,4 @@ TEST_F(IPCMessageParameterTest, Sync) { EXPECT_EQ(output, std::string("out")); }*/ -} // namespace +} // namespace IPC |