electron/patches/chromium/cherry-pick-1f8bec968902.patch

126 lines
5.3 KiB
Diff

From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Tsuyoshi Horo <horo@chromium.org>
Date: Wed, 24 Jan 2024 02:04:24 +0000
Subject: Fix UAF in SourceStreamToDataPipe
SourceStreamToDataPipe::ReadMore() is passing a callback with
Unretained(this) to net::SourceStream::Read(). But this callback may be
called even after the SourceStream is destructed. This is causing UAF
issue (crbug.com/1511085).
To solve this problem, this CL changes ReadMore() method to pass a
callback with a weak ptr of this.
(cherry picked from commit 6e36a69da1b73f9aea9c54bfbe6c5b9cb2c672a5)
Bug: 1511085
Change-Id: Idd4e34ff300ff5db2de1de7b303841c7db3a964a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5179746
Reviewed-by: Adam Rice <ricea@chromium.org>
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1244526}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5231558
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/branch-heads/6099@{#1860}
Cr-Branched-From: e6ee4500f7d6549a9ac1354f8d056da49ef406be-refs/heads/main@{#1217362}
diff --git a/services/network/public/cpp/source_stream_to_data_pipe.cc b/services/network/public/cpp/source_stream_to_data_pipe.cc
index bfd85b1a00b216b52ae816ca29cb66ddabe20b6d..07afd58a40f92485ded07c535092a891c5140c7b 100644
--- a/services/network/public/cpp/source_stream_to_data_pipe.cc
+++ b/services/network/public/cpp/source_stream_to_data_pipe.cc
@@ -55,9 +55,9 @@ void SourceStreamToDataPipe::ReadMore() {
scoped_refptr<net::IOBuffer> buffer(
new network::NetToMojoIOBuffer(pending_write_.get()));
- int result = source_->Read(
- buffer.get(), base::checked_cast<int>(num_bytes),
- base::BindOnce(&SourceStreamToDataPipe::DidRead, base::Unretained(this)));
+ int result = source_->Read(buffer.get(), base::checked_cast<int>(num_bytes),
+ base::BindOnce(&SourceStreamToDataPipe::DidRead,
+ weak_factory_.GetWeakPtr()));
if (result != net::ERR_IO_PENDING)
DidRead(result);
diff --git a/services/network/public/cpp/source_stream_to_data_pipe_unittest.cc b/services/network/public/cpp/source_stream_to_data_pipe_unittest.cc
index 7061418c5141d936f04b1193c98e66efc5e72ac5..54159df39afa7cf6e2faa51da185dc034b923209 100644
--- a/services/network/public/cpp/source_stream_to_data_pipe_unittest.cc
+++ b/services/network/public/cpp/source_stream_to_data_pipe_unittest.cc
@@ -6,7 +6,9 @@
#include "base/functional/bind.h"
#include "base/memory/raw_ptr.h"
+#include "base/test/bind.h"
#include "base/test/task_environment.h"
+#include "net/base/net_errors.h"
#include "net/filter/mock_source_stream.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/abseil-cpp/absl/types/optional.h"
@@ -42,6 +44,33 @@ struct SourceStreamToDataPipeTestParam {
const ReadResultType read_result_type;
};
+class DummyPendingSourceStream : public net::SourceStream {
+ public:
+ DummyPendingSourceStream() : net::SourceStream(SourceStream::TYPE_NONE) {}
+ ~DummyPendingSourceStream() override = default;
+
+ DummyPendingSourceStream(const DummyPendingSourceStream&) = delete;
+ DummyPendingSourceStream& operator=(const DummyPendingSourceStream&) = delete;
+
+ // SourceStream implementation
+ int Read(net::IOBuffer* dest_buffer,
+ int buffer_size,
+ net::CompletionOnceCallback callback) override {
+ callback_ = std::move(callback);
+ return net::ERR_IO_PENDING;
+ }
+ std::string Description() const override { return ""; }
+ bool MayHaveMoreBytes() const override { return true; }
+
+ net::CompletionOnceCallback TakeCompletionCallback() {
+ CHECK(callback_);
+ return std::move(callback_);
+ }
+
+ private:
+ net::CompletionOnceCallback callback_;
+};
+
} // namespace
class SourceStreamToDataPipeTest
@@ -212,4 +241,33 @@ TEST_P(SourceStreamToDataPipeTest, MayHaveMoreBytes) {
EXPECT_EQ(ReadPipe(&output), net::OK);
EXPECT_EQ(output, message);
}
+
+TEST(SourceStreamToDataPipeCallbackTest, CompletionCallbackAfterDestructed) {
+ base::test::TaskEnvironment task_environment;
+
+ std::unique_ptr<DummyPendingSourceStream> source =
+ std::make_unique<DummyPendingSourceStream>();
+ DummyPendingSourceStream* source_ptr = source.get();
+ const MojoCreateDataPipeOptions data_pipe_options{
+ sizeof(MojoCreateDataPipeOptions), MOJO_CREATE_DATA_PIPE_FLAG_NONE, 1, 1};
+ mojo::ScopedDataPipeProducerHandle producer_end;
+ mojo::ScopedDataPipeConsumerHandle consumer_end;
+ CHECK_EQ(MOJO_RESULT_OK, mojo::CreateDataPipe(&data_pipe_options,
+ producer_end, consumer_end));
+
+ std::unique_ptr<SourceStreamToDataPipe> adapter =
+ std::make_unique<SourceStreamToDataPipe>(std::move(source),
+ std::move(producer_end));
+ bool callback_called = false;
+ adapter->Start(
+ base::BindLambdaForTesting([&](int result) { callback_called = true; }));
+ net::CompletionOnceCallback callback = source_ptr->TakeCompletionCallback();
+ adapter.reset();
+
+ // Test that calling `callback` after deleting `adapter` must not cause UAF
+ // (crbug.com/1511085).
+ std::move(callback).Run(net::ERR_FAILED);
+ EXPECT_FALSE(callback_called);
+}
+
} // namespace network