zqz/platform-external-webrtc
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix potential unsafe access to VCMTimestampMap::data

Browse Source 
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}
This commit is contained in:
Johannes Kron
2021-03-03 14:39:44 +01:00
committed by Commit Bot
parent 752cbaba90
commit b6b782da68
5 changed files with 44 additions and 41 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
modules/video_coding/BUILD.gn
View File

@ -171,6 +171,7 @@ rtc_library("video_coding") {
"../../api:sequence_checker", "../../api:sequence_checker",
"../../api/units:data_rate", "../../api/units:data_rate",
"../../api/units:time_delta", "../../api/units:time_delta",
"../../api/units:timestamp",
"../../api/video:builtin_video_bitrate_allocator_factory", "../../api/video:builtin_video_bitrate_allocator_factory",
"../../api/video:encoded_frame", "../../api/video:encoded_frame",
"../../api/video:encoded_image", "../../api/video:encoded_image",

31
modules/video_coding/generic_decoder.cc
View File

@ -91,7 +91,7 @@ void VCMDecodedFrameCallback::Decoded(VideoFrame& decodedImage,
"timestamp", decodedImage.timestamp()); "timestamp", decodedImage.timestamp());
// TODO(holmer): We should improve this so that we can handle multiple // TODO(holmer): We should improve this so that we can handle multiple
// callbacks from one call to Decode(). // callbacks from one call to Decode().
VCMFrameInformation* frameInfo; absl::optional<VCMFrameInformation> frameInfo;
int timestamp_map_size = 0; int timestamp_map_size = 0;
{ {
MutexLock lock(&lock_); MutexLock lock(&lock_);
@ -99,7 +99,7 @@ void VCMDecodedFrameCallback::Decoded(VideoFrame& decodedImage,
timestamp_map_size = _timestampMap.Size(); timestamp_map_size = _timestampMap.Size();
} }
if (frameInfo == NULL) { if (!frameInfo) {
RTC_LOG(LS_WARNING) << "Too many frames backed up in the decoder, dropping " RTC_LOG(LS_WARNING) << "Too many frames backed up in the decoder, dropping "
"this one."; "this one.";
_receiveCallback->OnDroppedFrames(1); _receiveCallback->OnDroppedFrames(1);
@ -196,14 +196,14 @@ void VCMDecodedFrameCallback::OnDecoderImplementationName(
} }
void VCMDecodedFrameCallback::Map(uint32_t timestamp, void VCMDecodedFrameCallback::Map(uint32_t timestamp,
VCMFrameInformation* frameInfo) { const VCMFrameInformation& frameInfo) {
MutexLock lock(&lock_); MutexLock lock(&lock_);
_timestampMap.Add(timestamp, frameInfo); _timestampMap.Add(timestamp, frameInfo);
} }
int32_t VCMDecodedFrameCallback::Pop(uint32_t timestamp) { int32_t VCMDecodedFrameCallback::Pop(uint32_t timestamp) {
MutexLock lock(&lock_); MutexLock lock(&lock_);
if (_timestampMap.Pop(timestamp) == NULL) { if (_timestampMap.Pop(timestamp) == absl::nullopt) {
return VCM_GENERAL_ERROR; return VCM_GENERAL_ERROR;
} }
_receiveCallback->OnDroppedFrames(1); _receiveCallback->OnDroppedFrames(1);
@ -215,8 +215,6 @@ VCMGenericDecoder::VCMGenericDecoder(std::unique_ptr<VideoDecoder> decoder)
VCMGenericDecoder::VCMGenericDecoder(VideoDecoder* decoder, bool isExternal) VCMGenericDecoder::VCMGenericDecoder(VideoDecoder* decoder, bool isExternal)
: _callback(NULL), : _callback(NULL),
_frameInfos(),
_nextFrameInfoIdx(0),
decoder_(decoder), decoder_(decoder),
_codecType(kVideoCodecGeneric), _codecType(kVideoCodecGeneric),
_isExternal(isExternal), _isExternal(isExternal),
@ -249,26 +247,25 @@ int32_t VCMGenericDecoder::InitDecode(const VideoCodec* settings,
int32_t VCMGenericDecoder::Decode(const VCMEncodedFrame& frame, Timestamp now) { int32_t VCMGenericDecoder::Decode(const VCMEncodedFrame& frame, Timestamp now) {
TRACE_EVENT1("webrtc", "VCMGenericDecoder::Decode", "timestamp", TRACE_EVENT1("webrtc", "VCMGenericDecoder::Decode", "timestamp",
frame.Timestamp()); frame.Timestamp());
_frameInfos[_nextFrameInfoIdx].decodeStart = now; VCMFrameInformation frame_info;
_frameInfos[_nextFrameInfoIdx].renderTimeMs = frame.RenderTimeMs(); frame_info.decodeStart = now;
_frameInfos[_nextFrameInfoIdx].rotation = frame.rotation(); frame_info.renderTimeMs = frame.RenderTimeMs();
_frameInfos[_nextFrameInfoIdx].timing = frame.video_timing(); frame_info.rotation = frame.rotation();
_frameInfos[_nextFrameInfoIdx].ntp_time_ms = frame_info.timing = frame.video_timing();
frame.EncodedImage().ntp_time_ms_; frame_info.ntp_time_ms = frame.EncodedImage().ntp_time_ms_;
_frameInfos[_nextFrameInfoIdx].packet_infos = frame.PacketInfos(); frame_info.packet_infos = frame.PacketInfos();
// Set correctly only for key frames. Thus, use latest key frame // Set correctly only for key frames. Thus, use latest key frame
// content type. If the corresponding key frame was lost, decode will fail // content type. If the corresponding key frame was lost, decode will fail
// and content type will be ignored. // and content type will be ignored.
if (frame.FrameType() == VideoFrameType::kVideoFrameKey) { if (frame.FrameType() == VideoFrameType::kVideoFrameKey) {
_frameInfos[_nextFrameInfoIdx].content_type = frame.contentType(); frame_info.content_type = frame.contentType();
_last_keyframe_content_type = frame.contentType(); _last_keyframe_content_type = frame.contentType();
} else { } else {
_frameInfos[_nextFrameInfoIdx].content_type = _last_keyframe_content_type; frame_info.content_type = _last_keyframe_content_type;
} }
_callback->Map(frame.Timestamp(), &_frameInfos[_nextFrameInfoIdx]); _callback->Map(frame.Timestamp(), frame_info);
_nextFrameInfoIdx = (_nextFrameInfoIdx + 1) % kDecoderFrameMemoryLength;
int32_t ret = decoder_->Decode(frame.EncodedImage(), frame.MissingFrame(), int32_t ret = decoder_->Decode(frame.EncodedImage(), frame.MissingFrame(),
frame.RenderTimeMs()); frame.RenderTimeMs());
VideoDecoder::DecoderInfo decoder_info = decoder_->GetDecoderInfo(); VideoDecoder::DecoderInfo decoder_info = decoder_->GetDecoderInfo();

15
modules/video_coding/generic_decoder.h
View File

@ -29,18 +29,6 @@ class VCMReceiveCallback;
enum { kDecoderFrameMemoryLength = 10 }; enum { kDecoderFrameMemoryLength = 10 };
struct VCMFrameInformation {
int64_t renderTimeMs;
absl::optional<Timestamp> decodeStart;
void* userData;
VideoRotation rotation;
VideoContentType content_type;
EncodedImage::Timing timing;
int64_t ntp_time_ms;
RtpPacketInfos packet_infos;
// ColorSpace is not stored here, as it might be modified by decoders.
};
class VCMDecodedFrameCallback : public DecodedImageCallback { class VCMDecodedFrameCallback : public DecodedImageCallback {
public: public:
VCMDecodedFrameCallback(VCMTiming* timing, Clock* clock); VCMDecodedFrameCallback(VCMTiming* timing, Clock* clock);
@ -56,7 +44,7 @@ class VCMDecodedFrameCallback : public DecodedImageCallback {
void OnDecoderImplementationName(const char* implementation_name); void OnDecoderImplementationName(const char* implementation_name);
void Map(uint32_t timestamp, VCMFrameInformation* frameInfo); void Map(uint32_t timestamp, const VCMFrameInformation& frameInfo);
int32_t Pop(uint32_t timestamp); int32_t Pop(uint32_t timestamp);
private: private:
@ -117,7 +105,6 @@ class VCMGenericDecoder {
private: private:
VCMDecodedFrameCallback* _callback; VCMDecodedFrameCallback* _callback;
VCMFrameInformation _frameInfos[kDecoderFrameMemoryLength]; VCMFrameInformation _frameInfos[kDecoderFrameMemoryLength];
uint32_t _nextFrameInfoIdx;
std::unique_ptr<VideoDecoder> decoder_; std::unique_ptr<VideoDecoder> decoder_;
VideoCodecType _codecType; VideoCodecType _codecType;
const bool _isExternal; const bool _isExternal;

12
modules/video_coding/timestamp_map.cc
View File

@ -24,7 +24,7 @@ VCMTimestampMap::VCMTimestampMap(size_t capacity)
VCMTimestampMap::~VCMTimestampMap() {} 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_].timestamp = timestamp;
ring_buffer_[next_add_idx_].data = data; ring_buffer_[next_add_idx_].data = data;
next_add_idx_ = (next_add_idx_ + 1) % capacity_; next_add_idx_ = (next_add_idx_ + 1) % capacity_;
@ -35,18 +35,18 @@ void VCMTimestampMap::Add(uint32_t timestamp, VCMFrameInformation* data) {
} }
} }
VCMFrameInformation* VCMTimestampMap::Pop(uint32_t timestamp) { absl::optional<VCMFrameInformation> VCMTimestampMap::Pop(uint32_t timestamp) {
while (!IsEmpty()) { while (!IsEmpty()) {
if (ring_buffer_[next_pop_idx_].timestamp == timestamp) { if (ring_buffer_[next_pop_idx_].timestamp == timestamp) {
// Found start time for this timestamp. // Found start time for this timestamp.
VCMFrameInformation* data = ring_buffer_[next_pop_idx_].data; const VCMFrameInformation& data = ring_buffer_[next_pop_idx_].data;
ring_buffer_[next_pop_idx_].data = nullptr; ring_buffer_[next_pop_idx_].timestamp = 0;
next_pop_idx_ = (next_pop_idx_ + 1) % capacity_; next_pop_idx_ = (next_pop_idx_ + 1) % capacity_;
return data; return data;
} else if (IsNewerTimestamp(ring_buffer_[next_pop_idx_].timestamp, } else if (IsNewerTimestamp(ring_buffer_[next_pop_idx_].timestamp,
timestamp)) { timestamp)) {
// The timestamp we are looking for is not in the list. // 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). // Not in this position, check next (and forget this position).
@ -54,7 +54,7 @@ VCMFrameInformation* VCMTimestampMap::Pop(uint32_t timestamp) {
} }
// Could not find matching timestamp in list. // Could not find matching timestamp in list.
return nullptr; return absl::nullopt;
} }
bool VCMTimestampMap::IsEmpty() const { bool VCMTimestampMap::IsEmpty() const {

26
modules/video_coding/timestamp_map.h
View File

@ -13,23 +13,41 @@
#include <memory> #include <memory>
#include "absl/types/optional.h"
#include "api/rtp_packet_infos.h"
#include "api/units/timestamp.h"
#include "api/video/encoded_image.h"
#include "api/video/video_content_type.h"
#include "api/video/video_rotation.h"
#include "api/video/video_timing.h"
namespace webrtc { namespace webrtc {
struct VCMFrameInformation; struct VCMFrameInformation {
int64_t renderTimeMs;
absl::optional<Timestamp> decodeStart;
void* userData;
VideoRotation rotation;
VideoContentType content_type;
EncodedImage::Timing timing;
int64_t ntp_time_ms;
RtpPacketInfos packet_infos;
// ColorSpace is not stored here, as it might be modified by decoders.
};
class VCMTimestampMap { class VCMTimestampMap {
public: public:
explicit VCMTimestampMap(size_t capacity); explicit VCMTimestampMap(size_t capacity);
~VCMTimestampMap(); ~VCMTimestampMap();
void Add(uint32_t timestamp, VCMFrameInformation* data); void Add(uint32_t timestamp, const VCMFrameInformation& data);
VCMFrameInformation* Pop(uint32_t timestamp); absl::optional<VCMFrameInformation> Pop(uint32_t timestamp);
size_t Size() const; size_t Size() const;
private: private:
struct TimestampDataTuple { struct TimestampDataTuple {
uint32_t timestamp; uint32_t timestamp;
VCMFrameInformation* data; VCMFrameInformation data;
}; };
bool IsEmpty() const; bool IsEmpty() const;