Fix potential unsafe access to VCMTimestampMap::data
The access to |_timestampMap| was guarded by a lock but
not the access to the data pointer stored in |_timestampMap|.
There was a potential race condition if new data was added
in VCMGenericDecoder::Decode() while the data pointer
retrieved from _timestampMap.Pop() was being used in
VCMDecodedFrameCallback::Decoded().
This CL moves the storage of data to within |_timestampMap|,
instead of being a pointer so that it's guarded by the same
lock.
Bug: webrtc:11229
Change-Id: I3f2afb568ed724db5719d508a73de402c4531dec
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/209361
Commit-Queue: Johannes Kron <kron@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33374}
diff --git a/modules/video_coding/timestamp_map.cc b/modules/video_coding/timestamp_map.cc
index d79075f..7762e36 100644
--- a/modules/video_coding/timestamp_map.cc
+++ b/modules/video_coding/timestamp_map.cc
@@ -24,7 +24,7 @@
VCMTimestampMap::~VCMTimestampMap() {}
-void VCMTimestampMap::Add(uint32_t timestamp, VCMFrameInformation* data) {
+void VCMTimestampMap::Add(uint32_t timestamp, const VCMFrameInformation& data) {
ring_buffer_[next_add_idx_].timestamp = timestamp;
ring_buffer_[next_add_idx_].data = data;
next_add_idx_ = (next_add_idx_ + 1) % capacity_;
@@ -35,18 +35,18 @@
}
}
-VCMFrameInformation* VCMTimestampMap::Pop(uint32_t timestamp) {
+absl::optional<VCMFrameInformation> VCMTimestampMap::Pop(uint32_t timestamp) {
while (!IsEmpty()) {
if (ring_buffer_[next_pop_idx_].timestamp == timestamp) {
// Found start time for this timestamp.
- VCMFrameInformation* data = ring_buffer_[next_pop_idx_].data;
- ring_buffer_[next_pop_idx_].data = nullptr;
+ const VCMFrameInformation& data = ring_buffer_[next_pop_idx_].data;
+ ring_buffer_[next_pop_idx_].timestamp = 0;
next_pop_idx_ = (next_pop_idx_ + 1) % capacity_;
return data;
} else if (IsNewerTimestamp(ring_buffer_[next_pop_idx_].timestamp,
timestamp)) {
// The timestamp we are looking for is not in the list.
- return nullptr;
+ return absl::nullopt;
}
// Not in this position, check next (and forget this position).
@@ -54,7 +54,7 @@
}
// Could not find matching timestamp in list.
- return nullptr;
+ return absl::nullopt;
}
bool VCMTimestampMap::IsEmpty() const {