Skip to content

Commit

Permalink
[CrOS MultiDevice] Handle missing/stale BeaconSeeds.
Browse files Browse the repository at this point in the history 
SecureChannel previously assumed that
BleServiceDataHelper::GenerateForegroundAdvertisement() would always
successfully return advertisement data. However, it currently returns a
nullptr if either the local device public key or the remote device ID
is invalid. In particular, the former can happen if a user keeps their
Chromebook offline for over a month straight, resulting in stale
BeaconSeeds.

This CL adds the missing nullptr check within BleAdvertiserImpl, as
well as a new associated delegate method, which is implemented within
BleConnectionManagerImpl.

Bug: 854452
Change-Id: Icc0675950c674a34553fc8ea1f0bc0e70eece123
Reviewed-on: https://chromium-review.googlesource.com/1175078
Commit-Queue: Kyle Qian <[email protected]>
Reviewed-by: Kyle Horimoto <[email protected]>
Cr-Commit-Position: refs/heads/master@{#586490}
  • Loading branch information
Kyle Qian authored and Commit Bot committed Aug 27, 2018
1 parent c06438a commit 175fdd0
Show file tree
Hide file tree
Showing 10 changed files with 409 additions and 95 deletions.
5 changes: 5 additions & 0 deletions chromeos/services/secure_channel/ble_advertiser.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@ void BleAdvertiser::NotifyAdvertisingSlotEnded(
replaced_by_higher_priority_advertisement);
}

void BleAdvertiser::NotifyFailureToGenerateAdvertisement(
const DeviceIdPair& device_id_pair) {
delegate_->OnFailureToGenerateAdvertisement(device_id_pair);
}

} // namespace secure_channel

} // namespace chromeos
13 changes: 11 additions & 2 deletions chromeos/services/secure_channel/ble_advertiser.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ class BleAdvertiser {
virtual void OnAdvertisingSlotEnded(
const DeviceIdPair& device_id_pair,
bool replaced_by_higher_priority_advertisement) = 0;

// Invoked when there is a failure to generate an advertisement for
// |device_id_pair|.
virtual void OnFailureToGenerateAdvertisement(
const DeviceIdPair& device_id_pair) = 0;
};

virtual ~BleAdvertiser();
Expand All @@ -35,7 +40,9 @@ class BleAdvertiser {
//
// Calling AddAdvertisementRequest() does not guarantee that this Chromebook
// will immediately begin advertising to the remote device; because BLE
// advertisements are a shared system resource, requests may be queued.
// advertisements are a shared system resource, requests may be queued. If no
// advertisement can be generated,
// Delegate::OnFailureToGenerateAdvertisement() will be invoked.
virtual void AddAdvertisementRequest(
const DeviceIdPair& request,
ConnectionPriority connection_priority) = 0;
Expand All @@ -49,12 +56,14 @@ class BleAdvertiser {
virtual void RemoveAdvertisementRequest(const DeviceIdPair& request) = 0;

protected:
BleAdvertiser(Delegate* delegate);
explicit BleAdvertiser(Delegate* delegate);

void NotifyAdvertisingSlotEnded(
const DeviceIdPair& device_id_pair,
bool replaced_by_higher_priority_advertisement);

void NotifyFailureToGenerateAdvertisement(const DeviceIdPair& device_id_pair);

private:
Delegate* delegate_;

Expand Down
80 changes: 68 additions & 12 deletions chromeos/services/secure_channel/ble_advertiser_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "base/memory/ptr_util.h"
#include "base/no_destructor.h"
#include "base/stl_util.h"
#include "base/threading/sequenced_task_runner_handle.h"
#include "base/timer/timer.h"
#include "chromeos/components/proximity_auth/logging/logging.h"
#include "chromeos/services/secure_channel/ble_service_data_helper.h"
Expand Down Expand Up @@ -57,27 +58,34 @@ std::unique_ptr<BleAdvertiser> BleAdvertiserImpl::Factory::BuildInstance(
Delegate* delegate,
BleServiceDataHelper* ble_service_data_helper,
BleSynchronizerBase* ble_synchronizer_base,
TimerFactory* timer_factory) {
TimerFactory* timer_factory,
scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner) {
return base::WrapUnique(new BleAdvertiserImpl(
delegate, ble_service_data_helper, ble_synchronizer_base, timer_factory));
delegate, ble_service_data_helper, ble_synchronizer_base, timer_factory,
sequenced_task_runner));
}

BleAdvertiserImpl::BleAdvertiserImpl(
Delegate* delegate,
BleServiceDataHelper* ble_service_data_helper,
BleSynchronizerBase* ble_synchronizer_base,
TimerFactory* timer_factory)
TimerFactory* timer_factory,
scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner)
: BleAdvertiser(delegate),
ble_service_data_helper_(ble_service_data_helper),
ble_synchronizer_base_(ble_synchronizer_base),
timer_factory_(timer_factory),
shared_resource_scheduler_(std::make_unique<SharedResourceScheduler>()) {}
sequenced_task_runner_(sequenced_task_runner),
shared_resource_scheduler_(std::make_unique<SharedResourceScheduler>()),
weak_factory_(this) {}

BleAdvertiserImpl::~BleAdvertiserImpl() = default;

void BleAdvertiserImpl::AddAdvertisementRequest(
const DeviceIdPair& request,
ConnectionPriority connection_priority) {
requests_already_removed_due_to_failed_advertisement_.erase(request);

if (base::ContainsKey(all_requests_, request)) {
PA_LOG(ERROR) << "BleAdvertiserImpl::AddAdvertisementRequest(): Tried to "
<< "add advertisement request which was already present. "
Expand All @@ -102,6 +110,10 @@ void BleAdvertiserImpl::AddAdvertisementRequest(
void BleAdvertiserImpl::UpdateAdvertisementRequestPriority(
const DeviceIdPair& request,
ConnectionPriority connection_priority) {
if (base::ContainsKey(requests_already_removed_due_to_failed_advertisement_,
request))
return;

if (!base::ContainsKey(all_requests_, request)) {
PA_LOG(ERROR) << "BleAdvertiserImpl::UpdateAdvertisementRequestPriority(): "
<< "Tried to update request priority for a request, but that "
Expand Down Expand Up @@ -155,6 +167,14 @@ void BleAdvertiserImpl::UpdateAdvertisementRequestPriority(

void BleAdvertiserImpl::RemoveAdvertisementRequest(
const DeviceIdPair& request) {
// If the request has already been deleted, then this was invoked by a failure
// callback following a failure to generate an advertisement.
auto it = requests_already_removed_due_to_failed_advertisement_.find(request);
if (it != requests_already_removed_due_to_failed_advertisement_.end()) {
requests_already_removed_due_to_failed_advertisement_.erase(it);
return;
}

if (!base::ContainsKey(all_requests_, request)) {
PA_LOG(ERROR) << "BleAdvertiserImpl::RemoveAdvertisementRequest(): Tried "
<< "to remove an advertisement request, but that request was "
Expand Down Expand Up @@ -242,7 +262,7 @@ void BleAdvertiserImpl::UpdateAdvertisementState() {
// |active_advertisement_requests_| contains a request for an advertisement,
// generate a new active advertisement.
if (active_advertisement_requests_[i] && !active_advertisements_[i])
AddActiveAdvertisement(i);
AttemptToAddActiveAdvertisement(i);
}
}

Expand Down Expand Up @@ -270,13 +290,35 @@ void BleAdvertiserImpl::AddActiveAdvertisementRequest(size_t index_to_add) {
std::move(timer));
}

void BleAdvertiserImpl::AddActiveAdvertisement(size_t index_to_add) {
void BleAdvertiserImpl::AttemptToAddActiveAdvertisement(size_t index_to_add) {
const DeviceIdPair pair =
active_advertisement_requests_[index_to_add]->device_id_pair;
std::unique_ptr<cryptauth::DataWithTimestamp> service_data =
ble_service_data_helper_->GenerateForegroundAdvertisement(pair);

// If an advertisement could not be created, the request is immediately
// removed. It's also tracked to prevent future operations from referencing
// the removed request.
if (!service_data) {
RemoveAdvertisementRequest(pair);
requests_already_removed_due_to_failed_advertisement_.insert(pair);

// Schedules AttemptToNotifyFailureToGenerateAdvertisement() to run
// after the tasks in the current sequence. This is done to avoid invoking
// an advertisement generation failure callback on the same call stack that
// added the advertisement request in the first place.
sequenced_task_runner_->PostTask(
FROM_HERE,
base::BindOnce(
&BleAdvertiserImpl::AttemptToNotifyFailureToGenerateAdvertisement,
weak_factory_.GetWeakPtr(), pair));

return;
}

active_advertisements_[index_to_add] =
ErrorTolerantBleAdvertisementImpl::Factory::Get()->BuildInstance(
active_advertisement_requests_[index_to_add]->device_id_pair,
ble_service_data_helper_->GenerateForegroundAdvertisement(
active_advertisement_requests_[index_to_add]->device_id_pair),
ble_synchronizer_base_);
pair, std::move(service_data), ble_synchronizer_base_);
}

base::Optional<size_t> BleAdvertiserImpl::GetIndexForActiveRequest(
Expand All @@ -294,7 +336,7 @@ void BleAdvertiserImpl::StopAdvertisementRequestAndUpdateActiveRequests(
size_t index,
bool replaced_by_higher_priority_advertisement,
bool was_removed) {
// Stop the actual advertisement at this index.
// Stop the actual advertisement at this index, if there is one.
StopActiveAdvertisement(index);

// Make a copy of the request to stop from |active_advertisement_requests_|,
Expand All @@ -318,7 +360,8 @@ void BleAdvertiserImpl::StopAdvertisementRequestAndUpdateActiveRequests(

void BleAdvertiserImpl::StopActiveAdvertisement(size_t index) {
auto& active_advertisement = active_advertisements_[index];
DCHECK(active_advertisement);
if (!active_advertisement)
return;

// If |active_advertisement| is already in the process of stopping, there is
// nothing to do.
Expand All @@ -335,6 +378,19 @@ void BleAdvertiserImpl::OnActiveAdvertisementStopped(size_t index) {
UpdateAdvertisementState();
}

void BleAdvertiserImpl::AttemptToNotifyFailureToGenerateAdvertisement(
const DeviceIdPair& device_id_pair) {
// If the request is not found, then that request has either been removed
// again or re-scheduled after it failed to generate an advertisement, but
// before this task could execute.
if (!base::ContainsKey(requests_already_removed_due_to_failed_advertisement_,
device_id_pair)) {
return;
}

NotifyFailureToGenerateAdvertisement(device_id_pair);
}

} // namespace secure_channel

} // namespace chromeos
38 changes: 32 additions & 6 deletions chromeos/services/secure_channel/ble_advertiser_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,12 @@
#include <memory>
#include <utility>

#include "base/containers/flat_set.h"
#include "base/macros.h"
#include "base/memory/weak_ptr.h"
#include "base/optional.h"
#include "base/sequenced_task_runner.h"
#include "base/threading/sequenced_task_runner_handle.h"
#include "chromeos/services/secure_channel/ble_advertiser.h"
#include "chromeos/services/secure_channel/ble_constants.h"
#include "chromeos/services/secure_channel/device_id_pair.h"
Expand Down Expand Up @@ -57,7 +61,9 @@ class BleAdvertiserImpl : public BleAdvertiser {
Delegate* delegate,
BleServiceDataHelper* ble_service_data_helper,
BleSynchronizerBase* ble_synchronizer_base,
TimerFactory* timer_factory);
TimerFactory* timer_factory,
scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner =
base::SequencedTaskRunnerHandle::Get());

private:
static Factory* test_factory_;
Expand All @@ -83,10 +89,12 @@ class BleAdvertiserImpl : public BleAdvertiser {

static const int64_t kNumSecondsPerAdvertisementTimeslot;

BleAdvertiserImpl(Delegate* delegate,
BleServiceDataHelper* ble_service_data_helper,
BleSynchronizerBase* ble_synchronizer_base,
TimerFactory* timer_factory);
BleAdvertiserImpl(
Delegate* delegate,
BleServiceDataHelper* ble_service_data_helper,
BleSynchronizerBase* ble_synchronizer_base,
TimerFactory* timer_factory,
scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner);

// BleAdvertiser:
void AddAdvertisementRequest(const DeviceIdPair& request,
Expand All @@ -102,7 +110,7 @@ class BleAdvertiserImpl : public BleAdvertiser {
ConnectionPriority connection_priority);
void UpdateAdvertisementState();
void AddActiveAdvertisementRequest(size_t index_to_add);
void AddActiveAdvertisement(size_t index_to_add);
void AttemptToAddActiveAdvertisement(size_t index_to_add);
base::Optional<size_t> GetIndexForActiveRequest(const DeviceIdPair& request);
void StopAdvertisementRequestAndUpdateActiveRequests(
size_t index,
Expand All @@ -111,10 +119,19 @@ class BleAdvertiserImpl : public BleAdvertiser {
void StopActiveAdvertisement(size_t index);
void OnActiveAdvertisementStopped(size_t index);

// Notifies the delegate of a request's failure to generate an advertisement,
// unless the failed request has already been processed and removed from
// |requests_already_removed_due_to_failed_advertisement_|.
void AttemptToNotifyFailureToGenerateAdvertisement(
const DeviceIdPair& device_id_pair);

BleServiceDataHelper* ble_service_data_helper_;
BleSynchronizerBase* ble_synchronizer_base_;
TimerFactory* timer_factory_;

// For posting tasks to the current base::SequencedTaskRunner.
scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner_;

std::unique_ptr<SharedResourceScheduler> shared_resource_scheduler_;
DeviceIdPairSet all_requests_;

Expand All @@ -133,6 +150,15 @@ class BleAdvertiserImpl : public BleAdvertiser {
kMaxConcurrentAdvertisements>
active_advertisements_;

// If a request fails to generate an advertisement, it is immediately removed
// internally and tracked here. Then, when the delegate failure callback tries
// to clean up the failed advertisement (or something else tries to re-add or
// remove it again), its associated entry in this set will be removed instead.
base::flat_set<DeviceIdPair>
requests_already_removed_due_to_failed_advertisement_;

base::WeakPtrFactory<BleAdvertiserImpl> weak_factory_;

DISALLOW_COPY_AND_ASSIGN(BleAdvertiserImpl);
};

Expand Down
Loading

0 comments on commit 175fdd0

Please sign in to comment.