[Open Screen] Fix two unitialized reads dicovered by MSAN.
- Fix read of unitialized watch duration in OSP
- Fix uninitialized endpoint_id in OSP
Also, update Subscription to follow rule of 3/5, and simplify how
query IDs are allocated in the cast_app_discovery_service.
Bug: chromium:1143946
Change-Id: I78f3dd630626eaf47930d8a5c9bfdc3d05b49fb6
Reviewed-on: https://chromium-review.googlesource.com/c/openscreen/+/2511074
Commit-Queue: mark a. foltz <mfoltz@chromium.org>
Reviewed-by: Brandon Tolsch <btolsch@chromium.org>
diff --git a/cast/sender/cast_app_discovery_service_impl.cc b/cast/sender/cast_app_discovery_service_impl.cc
index 955ab0e..fd071ca 100644
--- a/cast/sender/cast_app_discovery_service_impl.cc
+++ b/cast/sender/cast_app_discovery_service_impl.cc
@@ -6,6 +6,7 @@
#include <algorithm>
#include <chrono>
+#include <utility>
#include "cast/sender/public/cast_media_source.h"
#include "util/osp_logging.h"
@@ -47,7 +48,7 @@
}
auto& callbacks = avail_queries_[source_id];
- uint32_t query_id = GetNextAvailabilityQueryId();
+ uint32_t query_id = next_avail_query_id_++;
callbacks.push_back({query_id, std::move(callback)});
if (callbacks.size() == 1) {
// NOTE: Even though we retain availability results for an app unregistered
@@ -62,7 +63,7 @@
}
}
- return MakeSubscription(this, query_id);
+ return Subscription(this, query_id);
}
void CastAppDiscoveryServiceImpl::Refresh() {
@@ -176,16 +177,6 @@
return false;
}
-uint32_t CastAppDiscoveryServiceImpl::GetNextAvailabilityQueryId() {
- if (free_query_ids_.empty()) {
- return next_avail_query_id_++;
- } else {
- uint32_t id = free_query_ids_.back();
- free_query_ids_.pop_back();
- return id;
- }
-}
-
void CastAppDiscoveryServiceImpl::RemoveAvailabilityCallback(uint32_t id) {
for (auto entry = avail_queries_.begin(); entry != avail_queries_.end();
++entry) {
diff --git a/cast/sender/cast_app_discovery_service_impl.h b/cast/sender/cast_app_discovery_service_impl.h
index 4093311..fa57780 100644
--- a/cast/sender/cast_app_discovery_service_impl.h
+++ b/cast/sender/cast_app_discovery_service_impl.h
@@ -70,8 +70,6 @@
const std::string& app_id,
Clock::time_point now) const;
- uint32_t GetNextAvailabilityQueryId();
-
void RemoveAvailabilityCallback(uint32_t id) override;
std::map<std::string, ServiceInfo> receivers_by_id_;
@@ -81,8 +79,7 @@
std::map<std::string, std::vector<AvailabilityCallbackEntry>> avail_queries_;
// Callback ID tracking.
- uint32_t next_avail_query_id_;
- std::vector<uint32_t> free_query_ids_;
+ uint32_t next_avail_query_id_ = 1U;
CastPlatformClient* const platform_client_;
diff --git a/cast/sender/public/cast_app_discovery_service.cc b/cast/sender/public/cast_app_discovery_service.cc
index c561b38..8299aff 100644
--- a/cast/sender/public/cast_app_discovery_service.cc
+++ b/cast/sender/public/cast_app_discovery_service.cc
@@ -17,14 +17,16 @@
other.discovery_service_ = nullptr;
}
-CastAppDiscoveryService::Subscription::~Subscription() {
- Reset();
+CastAppDiscoveryService::Subscription&
+CastAppDiscoveryService::Subscription::operator=(Subscription&& other) {
+ id_ = other.id_;
+ discovery_service_ = other.discovery_service_;
+ other.discovery_service_ = nullptr;
+ return *this;
}
-CastAppDiscoveryService::Subscription& CastAppDiscoveryService::Subscription::
-operator=(Subscription other) {
- Swap(other);
- return *this;
+CastAppDiscoveryService::Subscription::~Subscription() {
+ Reset();
}
void CastAppDiscoveryService::Subscription::Reset() {
diff --git a/cast/sender/public/cast_app_discovery_service.h b/cast/sender/public/cast_app_discovery_service.h
index ccb586f..6ce5ee7 100644
--- a/cast/sender/public/cast_app_discovery_service.h
+++ b/cast/sender/public/cast_app_discovery_service.h
@@ -23,17 +23,18 @@
class Subscription {
public:
+ Subscription(CastAppDiscoveryService* discovery_service, uint32_t id);
Subscription(Subscription&&);
+ Subscription(Subscription&) = delete;
+ Subscription& operator=(Subscription&&);
+ Subscription& operator=(const Subscription&) = delete;
~Subscription();
- Subscription& operator=(Subscription);
void Reset();
private:
friend class CastAppDiscoveryService;
- Subscription(CastAppDiscoveryService* discovery_service, uint32_t id);
-
void Swap(Subscription& other);
CastAppDiscoveryService* discovery_service_;
@@ -57,15 +58,7 @@
// this method when the user initiates a user gesture.
virtual void Refresh() = 0;
- protected:
- Subscription MakeSubscription(CastAppDiscoveryService* discovery_service,
- uint32_t id) {
- return Subscription(discovery_service, id);
- }
-
private:
- friend class Subscription;
-
virtual void RemoveAvailabilityCallback(uint32_t id) = 0;
};
diff --git a/osp/impl/presentation/url_availability_requester.cc b/osp/impl/presentation/url_availability_requester.cc
index 93c53b0..a7b2797 100644
--- a/osp/impl/presentation/url_availability_requester.cc
+++ b/osp/impl/presentation/url_availability_requester.cc
@@ -7,9 +7,12 @@
#include <algorithm>
#include <chrono>
#include <memory>
+#include <utility>
+#include <vector>
#include "osp/impl/presentation/presentation_common.h"
#include "osp/public/network_service_manager.h"
+#include "util/chrono_helpers.h"
#include "util/osp_logging.h"
using std::chrono::seconds;
@@ -224,6 +227,7 @@
cbor_request.request_id = request_id;
cbor_request.urls = urls;
cbor_request.watch_id = watch_id;
+ cbor_request.watch_duration = to_microseconds(kWatchDuration).count();
msgs::CborEncodeBuffer buffer;
if (msgs::EncodePresentationUrlAvailabilityRequest(cbor_request, &buffer)) {
diff --git a/osp/impl/presentation/url_availability_requester.h b/osp/impl/presentation/url_availability_requester.h
index dd0309a..ca32dd4 100644
--- a/osp/impl/presentation/url_availability_requester.h
+++ b/osp/impl/presentation/url_availability_requester.h
@@ -9,6 +9,7 @@
#include <memory>
#include <set>
#include <string>
+#include <vector>
#include "osp/msgs/osp_messages.h"
#include "osp/public/message_demuxer.h"
@@ -124,7 +125,7 @@
uint64_t next_watch_id = 1;
const std::string service_id;
- uint64_t endpoint_id;
+ uint64_t endpoint_id{0};
ProtocolConnectionClient::ConnectRequest connect_request;
// TODO(btolsch): Observe connection and restart all the things on close.
