Commit a5d25fbe authored by Sorin Jianu's avatar Sorin Jianu Committed by Commit Bot

Handle an ill formed response from UpdateClient::CrxDataCallback.

There could be executions path in the code where early returns in
the callback implementations return an empty vector instead of
a vector of CrxComponent of the same size as the input parameter
of the callback.

When this occurs, the original code indexes out of bounds and crashes.

With the fix, if the callback returns fewer values than the number of
components checked for updates, the UpdateClient::Update call is
rejected by the update engine, and completed with a specific error code.

Bug: 1144880
Change-Id: If45910c7c9ea0ca940b2d5a3d936b96a31d9c684
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2518001Reviewed-by: default avatarJoshua Pawlicki <waffles@chromium.org>
Commit-Queue: Sorin Jianu <sorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#823985}
parent 5449f59a
......@@ -103,7 +103,7 @@ void PingManagerTest::PingSentCallback(int error, const std::string& response) {
scoped_refptr<UpdateContext> PingManagerTest::MakeMockUpdateContext() const {
return base::MakeRefCounted<UpdateContext>(
config_, false, std::vector<std::string>(),
UpdateClient::CrxDataCallback(), UpdateClient::CrxStateChangeCallback(),
UpdateClient::CrxStateChangeCallback(),
UpdateEngine::NotifyObserversCallback(), UpdateEngine::Callback(),
nullptr);
}
......
......@@ -227,7 +227,7 @@ void UpdateCheckerTest::UpdateCheckComplete(
scoped_refptr<UpdateContext> UpdateCheckerTest::MakeMockUpdateContext() const {
return base::MakeRefCounted<UpdateContext>(
config_, false, std::vector<std::string>(),
UpdateClient::CrxDataCallback(), UpdateClient::CrxStateChangeCallback(),
UpdateClient::CrxStateChangeCallback(),
UpdateEngine::NotifyObserversCallback(), UpdateEngine::Callback(),
nullptr);
}
......
......@@ -331,6 +331,12 @@ using Callback = base::OnceCallback<void(Error error)>;
// the browser process has gone single-threaded.
class UpdateClient : public base::RefCountedThreadSafe<UpdateClient> {
public:
// Returns `CrxComponent` instances corresponding to the component ids
// passed as an argument to the callback. The order of components in the input
// and output vectors must match. If the instance of the `CrxComponent` is not
// available for some reason, implementors of the callback must not skip
// skip the component, and instead, they must insert a `nullopt` value in
// the output vector.
using CrxDataCallback =
base::OnceCallback<std::vector<base::Optional<CrxComponent>>(
const std::vector<std::string>& ids)>;
......
......@@ -18,6 +18,7 @@ enum class Error {
UPDATE_CHECK_ERROR = 5,
CRX_NOT_FOUND = 6,
INVALID_ARGUMENT = 7,
BAD_CRX_DATA_CALLBACK = 8,
MAX_VALUE,
};
......
......@@ -4587,4 +4587,60 @@ TEST_F(UpdateClientTest, CustomAttributeNoUpdate) {
EXPECT_EQ(1, observer.calls);
}
// Tests the scenario where `CrxDataCallback` returns a vector whose elements
// don't include a value for one of the component ids specified by the `ids`
// parameter of the `UpdateClient::Update` function. Expects the completion
// callback to include a specific error, and no other events and pings be
// generated, since the update engine rejects the UpdateClient::Update call.
TEST_F(UpdateClientTest, BadCrxDataCallback) {
class CompletionCallbackMock {
public:
static void Callback(base::OnceClosure quit_closure, Error error) {
EXPECT_EQ(Error::BAD_CRX_DATA_CALLBACK, error);
std::move(quit_closure).Run();
}
};
class MockPingManager : public MockPingManagerImpl {
public:
explicit MockPingManager(scoped_refptr<Configurator> config)
: MockPingManagerImpl(config) {}
protected:
~MockPingManager() override { EXPECT_TRUE(ping_data().empty()); }
};
scoped_refptr<UpdateClient> update_client =
base::MakeRefCounted<UpdateClientImpl>(
config(), base::MakeRefCounted<MockPingManager>(config()),
UpdateChecker::Factory{});
MockObserver observer;
std::vector<CrxUpdateItem> items;
auto receiver = base::MakeRefCounted<MockCrxStateChangeReceiver>();
EXPECT_CALL(*receiver, Receive(_))
.WillRepeatedly(
[&items](const CrxUpdateItem& item) { items.push_back(item); });
update_client->AddObserver(&observer);
const std::vector<std::string> ids = {"jebgalgnebhfojomionfpkfelancnnkf",
"gjpmebpgbhcamgdgjcmnjfhggjpgcimm"};
// The `CrxDataCallback` argument only returns a value for the first
// component id. This means that its result is ill formed, and the `Update`
// call completes with an error.
update_client->Update(
ids, base::BindOnce([](const std::vector<std::string>& ids) {
EXPECT_EQ(ids.size(), size_t{2});
return std::vector<base::Optional<CrxComponent>>{base::nullopt};
}),
base::BindRepeating(&MockCrxStateChangeReceiver::Receive, receiver), true,
base::BindOnce(&CompletionCallbackMock::Callback, quit_closure()));
RunThreads();
EXPECT_TRUE(items.empty());
update_client->RemoveObserver(&observer);
}
} // namespace update_client
......@@ -32,7 +32,6 @@ UpdateContext::UpdateContext(
scoped_refptr<Configurator> config,
bool is_foreground,
const std::vector<std::string>& ids,
UpdateClient::CrxDataCallback crx_data_callback,
UpdateClient::CrxStateChangeCallback crx_state_change_callback,
const UpdateEngine::NotifyObserversCallback& notify_observers_callback,
UpdateEngine::Callback callback,
......@@ -41,7 +40,6 @@ UpdateContext::UpdateContext(
is_foreground(is_foreground),
enabled_component_updates(config->EnabledComponentUpdates()),
ids(ids),
crx_data_callback(std::move(crx_data_callback)),
crx_state_change_callback(crx_state_change_callback),
notify_observers_callback(notify_observers_callback),
callback(std::move(callback)),
......@@ -93,22 +91,25 @@ void UpdateEngine::Update(
return;
}
// Calls out to get the corresponding CrxComponent data for the components.
const std::vector<base::Optional<CrxComponent>> crx_components =
std::move(crx_data_callback).Run(ids);
if (crx_components.size() < ids.size()) {
base::ThreadTaskRunnerHandle::Get()->PostTask(
FROM_HERE,
base::BindOnce(std::move(callback), Error::BAD_CRX_DATA_CALLBACK));
return;
}
const auto update_context = base::MakeRefCounted<UpdateContext>(
config_, is_foreground, ids, std::move(crx_data_callback),
crx_state_change_callback, notify_observers_callback_,
std::move(callback), metadata_.get());
config_, is_foreground, ids, crx_state_change_callback,
notify_observers_callback_, std::move(callback), metadata_.get());
DCHECK(!update_context->session_id.empty());
const auto result = update_contexts_.insert(
std::make_pair(update_context->session_id, update_context));
DCHECK(result.second);
// Calls out to get the corresponding CrxComponent data for the CRXs in this
// update context.
const auto crx_components =
std::move(update_context->crx_data_callback).Run(update_context->ids);
DCHECK_EQ(update_context->ids.size(), crx_components.size());
for (size_t i = 0; i != update_context->ids.size(); ++i) {
const auto& id = update_context->ids[i];
......@@ -407,7 +408,7 @@ void UpdateEngine::SendUninstallPing(const std::string& id,
const auto update_context = base::MakeRefCounted<UpdateContext>(
config_, false, std::vector<std::string>{id},
UpdateClient::CrxDataCallback(), UpdateClient::CrxStateChangeCallback(),
UpdateClient::CrxStateChangeCallback(),
UpdateEngine::NotifyObserversCallback(), std::move(callback),
metadata_.get());
DCHECK(!update_context->session_id.empty());
......@@ -437,7 +438,7 @@ void UpdateEngine::SendRegistrationPing(const std::string& id,
const auto update_context = base::MakeRefCounted<UpdateContext>(
config_, false, std::vector<std::string>{id},
UpdateClient::CrxDataCallback(), UpdateClient::CrxStateChangeCallback(),
UpdateClient::CrxStateChangeCallback(),
UpdateEngine::NotifyObserversCallback(), std::move(callback),
metadata_.get());
DCHECK(!update_context->session_id.empty());
......
......@@ -127,7 +127,6 @@ struct UpdateContext : public base::RefCountedThreadSafe<UpdateContext> {
scoped_refptr<Configurator> config,
bool is_foreground,
const std::vector<std::string>& ids,
UpdateClient::CrxDataCallback crx_data_callback,
UpdateClient::CrxStateChangeCallback crx_state_change_callback,
const UpdateEngine::NotifyObserversCallback& notify_observers_callback,
UpdateEngine::Callback callback,
......@@ -150,9 +149,6 @@ struct UpdateContext : public base::RefCountedThreadSafe<UpdateContext> {
// Contains the map of ids to components for all the CRX in this context.
IdToComponentPtrMap components;
// Called before an update check, when update metadata is needed.
UpdateEngine::CrxDataCallback crx_data_callback;
// Called when the observable state of the CRX component has changed.
UpdateClient::CrxStateChangeCallback crx_state_change_callback;
......
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