Skip to content

Commit

Permalink
Fix use after free in RTCVideoEncoder on encoder recreation
Browse files Browse the repository at this point in the history
In case WebRTC encoded video transform is used, encoded frame buffers
provided by RTCVideoEncoder are pushed onto worker. Thus, lifetime
of these buffers needs to be ensured when RTCVideoEncoder instance
is recreated.

BUG=1448809

(cherry picked from commit aee9c86)

Change-Id: Iff52541b6b4e3264caf985808ceb42d768d8ac1b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4565337
Reviewed-by: Tony Herre <toprice@chromium.org>
Reviewed-by: Harald Alvestrand <hta@chromium.org>
Commit-Queue: Martin Aedla <maedla@microsoft.com>
Cr-Original-Commit-Position: refs/heads/main@{#1150441}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4584792
Commit-Queue: Guido Urdaneta <guidou@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/5790@{#301}
Cr-Branched-From: 1d71a33-refs/heads/main@{#1148114}
  • Loading branch information
martinaedla authored and Chromium LUCI CQ committed Jun 3, 2023
1 parent 3bc64a7 commit 39822ea
Show file tree
Hide file tree
Showing 2 changed files with 122 additions and 15 deletions.
Expand Up @@ -203,24 +203,54 @@ class ScopedSignaledValue {
SignaledValue sv;
};

// TODO(https://crbug.com/1448809): Move to base/memory/ref_counted_memory.h
class RefCountedWritableSharedMemoryMapping
: public base::RefCountedThreadSafe<RefCountedWritableSharedMemoryMapping> {
public:
explicit RefCountedWritableSharedMemoryMapping(
base::WritableSharedMemoryMapping mapping)
: mapping_(std::move(mapping)) {}

RefCountedWritableSharedMemoryMapping(
const RefCountedWritableSharedMemoryMapping&) = delete;
RefCountedWritableSharedMemoryMapping& operator=(
const RefCountedWritableSharedMemoryMapping&) = delete;

const unsigned char* front() const {
return static_cast<unsigned char*>(mapping_.memory());
}
unsigned char* front() {
return static_cast<unsigned char*>(mapping_.memory());
}
size_t size() const { return mapping_.size(); }

private:
friend class base::RefCountedThreadSafe<
RefCountedWritableSharedMemoryMapping>;
~RefCountedWritableSharedMemoryMapping() = default;

const base::WritableSharedMemoryMapping mapping_;
};

class EncodedDataWrapper : public webrtc::EncodedImageBufferInterface {
public:
EncodedDataWrapper(uint8_t* data,
size_t size,
base::OnceClosure reuse_buffer_callback)
: data_(data),
EncodedDataWrapper(
const scoped_refptr<RefCountedWritableSharedMemoryMapping>&& mapping,
size_t size,
base::OnceClosure reuse_buffer_callback)
: mapping_(std::move(mapping)),
size_(size),
reuse_buffer_callback_(std::move(reuse_buffer_callback)) {}
~EncodedDataWrapper() override {
DCHECK(reuse_buffer_callback_);
std::move(reuse_buffer_callback_).Run();
}
const uint8_t* data() const override { return data_; }
uint8_t* data() override { return data_; }
const uint8_t* data() const override { return mapping_->front(); }
uint8_t* data() override { return mapping_->front(); }
size_t size() const override { return size_; }

private:
uint8_t* const data_;
const scoped_refptr<RefCountedWritableSharedMemoryMapping> mapping_;
const size_t size_;
base::OnceClosure reuse_buffer_callback_;
};
Expand Down Expand Up @@ -685,7 +715,7 @@ class RTCVideoEncoder::Impl : public media::VideoEncodeAccelerator::Client {
Vector<std::unique_ptr<base::MappedReadOnlyRegion>> input_buffers_;

Vector<std::pair<base::UnsafeSharedMemoryRegion,
base::WritableSharedMemoryMapping>>
scoped_refptr<RefCountedWritableSharedMemoryMapping>>>
output_buffers_;

// The number of frames that are sent to a hardware video encoder by Encode()
Expand Down Expand Up @@ -891,7 +921,6 @@ void RTCVideoEncoder::Impl::Enqueue(FrameChunk frame_chunk,
EncodeOneFrameWithNativeInput(std::move(frame_chunk));
return;
}

pending_frames_.push_back(std::move(frame_chunk));
// When |input_buffers_free_| is empty, EncodeOneFrame() for the frame in
// |pending_frames_| will be invoked from InputBufferReleased().
Expand Down Expand Up @@ -1034,8 +1063,10 @@ void RTCVideoEncoder::Impl::RequireBitstreamBuffers(
"failed to create output buffer"});
return;
}
output_buffers_.push_back(
std::make_pair(std::move(region), std::move(mapping)));
output_buffers_.push_back(std::make_pair(
std::move(region),
base::MakeRefCounted<RefCountedWritableSharedMemoryMapping>(
std::move(mapping))));
}

// Immediately provide all output buffers to the VEA.
Expand Down Expand Up @@ -1069,15 +1100,16 @@ void RTCVideoEncoder::Impl::BitstreamBufferReady(
base::NumberToString(bitstream_buffer_id)});
return;
}
void* output_mapping_memory =
output_buffers_[bitstream_buffer_id].second.memory();
scoped_refptr<RefCountedWritableSharedMemoryMapping> output_mapping =
output_buffers_[bitstream_buffer_id].second;
if (metadata.payload_size_bytes >
output_buffers_[bitstream_buffer_id].second.size()) {
output_buffers_[bitstream_buffer_id].second->size()) {
NotifyErrorStatus({media::EncoderStatus::Codes::kInvalidOutputBuffer,
"invalid payload_size: " +
base::NumberToString(metadata.payload_size_bytes)});
return;
}

DCHECK_NE(output_buffers_in_encoder_count_, 0u);
output_buffers_in_encoder_count_--;

Expand Down Expand Up @@ -1145,7 +1177,7 @@ void RTCVideoEncoder::Impl::BitstreamBufferReady(

webrtc::EncodedImage image;
image.SetEncodedData(rtc::make_ref_counted<EncodedDataWrapper>(
static_cast<uint8_t*>(output_mapping_memory), metadata.payload_size_bytes,
std::move(output_mapping), metadata.payload_size_bytes,
base::BindPostTaskToCurrentDefault(
base::BindOnce(&RTCVideoEncoder::Impl::BitstreamBufferAvailable,
weak_this_, bitstream_buffer_id))));
Expand Down
Expand Up @@ -1606,6 +1606,81 @@ TEST_P(RTCVideoEncoderEncodeTest, EncodeFrameWithAdapter) {
&frame_types));
}

TEST_P(RTCVideoEncoderEncodeTest, EncodedBufferLifetimeExceedsEncoderLifetime) {
webrtc::VideoCodec codec = GetSVCLayerCodec(webrtc::kVideoCodecVP9,
/*num_spatial_layers=*/1);
CreateEncoder(codec.codecType);

if (!InitializeOnFirstFrameEnabled()) {
ExpectCreateInitAndDestroyVEA();
}
EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK,
rtc_encoder_->InitEncode(&codec, kVideoEncoderSettings));

constexpr size_t kNumEncodeFrames = 3u;
class EnodedBufferLifetimeVerifier : public webrtc::EncodedImageCallback {
public:
explicit EnodedBufferLifetimeVerifier() = default;
~EnodedBufferLifetimeVerifier() override {
last_encoded_image_->data()[0] = 0;
}

webrtc::EncodedImageCallback::Result OnEncodedImage(
const webrtc::EncodedImage& encoded_image,
const webrtc::CodecSpecificInfo* codec_specific_info) override {
last_encoded_image_ = encoded_image.GetEncodedData();
if (encoded_image.Timestamp() == kNumEncodeFrames - 1 &&
codec_specific_info->end_of_picture) {
waiter_.Signal();
}
return Result(Result::OK);
}

void Wait() { waiter_.Wait(); }

private:
base::WaitableEvent waiter_;
rtc::scoped_refptr<webrtc::EncodedImageBufferInterface> last_encoded_image_;
};

EnodedBufferLifetimeVerifier lifetime_verifier;
rtc_encoder_->RegisterEncodeCompleteCallback(&lifetime_verifier);
if (InitializeOnFirstFrameEnabled()) {
ExpectCreateInitAndDestroyVEA();
}
for (size_t i = 0; i < kNumEncodeFrames; i++) {
const rtc::scoped_refptr<webrtc::I420Buffer> buffer =
webrtc::I420Buffer::Create(kInputFrameWidth, kInputFrameHeight);
FillFrameBuffer(buffer);
std::vector<webrtc::VideoFrameType> frame_types;
if (i == 0) {
frame_types.emplace_back(webrtc::VideoFrameType::kVideoFrameKey);
}
base::WaitableEvent event;
if (i > 0) {
EXPECT_CALL(*mock_vea_, UseOutputBitstreamBuffer(_))
.Times((i > 1) ? 1 : 0);
}
EXPECT_CALL(*mock_vea_, Encode)
.WillOnce(DoAll(
Invoke(this,
&RTCVideoEncoderTest::ReturnSVCLayerFrameWithVp9Metadata),
[&event]() { event.Signal(); }));
EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK,
rtc_encoder_->Encode(webrtc::VideoFrame::Builder()
.set_video_frame_buffer(buffer)
.set_timestamp_rtp(i)
.set_timestamp_us(i)
.set_rotation(webrtc::kVideoRotation_0)
.build(),
&frame_types));
event.Wait();
}
lifetime_verifier.Wait();
RunUntilIdle();
rtc_encoder_.reset();
}

const RTCVideoEncoderEncodeTestParam kEncodeTestCases[] = {
{false, false},
{false, true},
Expand Down

0 comments on commit 39822ea

Please sign in to comment.