Commit 8ad25003 authored by Brian Geffon's avatar Brian Geffon Committed by Commit Bot

Mojo: NodeChannel should require a minimum size when using multiple versions

When we added support for versioned message we accidentally introduced a
behavior where messages can be shorter than expected. This is what the
behavior should be to allow older messages to be received. However,
we know the minimum size is the oldest message version. We should
never be allowed to go smaller than that.

BUG=chromium:1149805

Change-Id: I3a24dd62b6fafd742cd7ac155f863d33bb5c6de8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2545207Reviewed-by: default avatarKen Rockot <rockot@google.com>
Commit-Queue: Brian Geffon <bgeffon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#828478}
parent bbd2cd14
...@@ -190,13 +190,16 @@ Channel::MessagePtr CreateMessage(MessageType type, ...@@ -190,13 +190,16 @@ Channel::MessagePtr CreateMessage(MessageType type,
return msg_ptr; return msg_ptr;
} }
template <typename DataType> // This method takes a second template argument which is another datatype which
bool GetMessagePayload(const void* bytes, // represents the smallest size this payload can be to be considered valid this
size_t num_bytes, // MUST be used when there is more than one version of a message to specify the
DataType* out_data) { // oldest version of the message.
template <typename DataType, typename MinSizedDataType>
bool GetMessagePayloadMinimumSized(const void* bytes,
size_t num_bytes,
DataType* out_data) {
static_assert(sizeof(DataType) > 0, "DataType must have non-zero size."); static_assert(sizeof(DataType) > 0, "DataType must have non-zero size.");
// We should have at least 1 byte to contribute towards DataType. if (num_bytes < sizeof(Header) + sizeof(MinSizedDataType))
if (num_bytes <= sizeof(Header))
return false; return false;
// Always make sure that the full object is zeored and default constructed as // Always make sure that the full object is zeored and default constructed as
...@@ -211,6 +214,14 @@ bool GetMessagePayload(const void* bytes, ...@@ -211,6 +214,14 @@ bool GetMessagePayload(const void* bytes,
return true; return true;
} }
template <typename DataType>
bool GetMessagePayload(const void* bytes,
size_t num_bytes,
DataType* out_data) {
return GetMessagePayloadMinimumSized<DataType, DataType>(bytes, num_bytes,
out_data);
}
} // namespace } // namespace
// static // static
......
...@@ -5,6 +5,7 @@ ...@@ -5,6 +5,7 @@
#include "mojo/core/node_channel.h" #include "mojo/core/node_channel.h"
#include "base/callback_helpers.h" #include "base/callback_helpers.h"
#include "base/logging.h"
#include "base/memory/scoped_refptr.h" #include "base/memory/scoped_refptr.h"
#include "base/message_loop/message_pump_type.h" #include "base/message_loop/message_pump_type.h"
#include "base/test/task_environment.h" #include "base/test/task_environment.h"
...@@ -21,6 +22,7 @@ namespace { ...@@ -21,6 +22,7 @@ namespace {
using NodeChannelTest = testing::Test; using NodeChannelTest = testing::Test;
using ports::NodeName; using ports::NodeName;
using testing::_;
scoped_refptr<NodeChannel> CreateNodeChannel(NodeChannel::Delegate* delegate, scoped_refptr<NodeChannel> CreateNodeChannel(NodeChannel::Delegate* delegate,
PlatformChannelEndpoint endpoint) { PlatformChannelEndpoint endpoint) {
...@@ -67,6 +69,48 @@ TEST_F(NodeChannelTest, DestructionIsSafe) { ...@@ -67,6 +69,48 @@ TEST_F(NodeChannelTest, DestructionIsSafe) {
error_loop.Run(); error_loop.Run();
} }
TEST_F(NodeChannelTest, MessagesCannotBeSmallerThanOldestVersion) {
base::test::TaskEnvironment task_environment;
PlatformChannel channel;
MockNodeChannelDelegate local_delegate;
auto local_channel =
CreateNodeChannel(&local_delegate, channel.TakeLocalEndpoint());
local_channel->Start();
MockNodeChannelDelegate remote_delegate;
auto remote_channel =
CreateNodeChannel(&remote_delegate, channel.TakeRemoteEndpoint());
remote_channel->Start();
base::RunLoop loop;
// It's a bad message and shouldn't be passed to the delegate.
EXPECT_CALL(local_delegate, OnRequestPortMerge(_, _, _)).Times(0);
// This good message should go through after.
const NodeName kRemoteNodeName{123, 456};
const NodeName kToken{987, 654};
EXPECT_CALL(local_delegate,
OnAcceptInvitee(ports::kInvalidNodeName, kRemoteNodeName, kToken))
.WillRepeatedly([&] {
loop.Quit(); });
// 1 byte is not enough to contain the oldest version of the request port
// merge payload, it should be discarded.
int payload_size = 1;
int capacity = /*sizeof(header)=*/8 + payload_size;
auto message =
std::make_unique<Channel::Message>(capacity, capacity, /*num_handles=*/0);
// Set the type of this message as REQUEST_PORT_MERGE (6)
*reinterpret_cast<uint32_t*>(message->mutable_payload()) = 6;
// This short message should be ignored.
remote_channel->SendChannelMessage(std::move(message));
remote_channel->AcceptInvitee(kRemoteNodeName, kToken);
loop.Run();
}
} // namespace } // namespace
} // namespace core } // namespace core
} // namespace mojo } // namespace mojo
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment