Use raw pointer instead of shared_ptr
Throughout update_engine, there are many places we pass a shared_ptr of
FileDescriptor, for memory safety. But in many cases, functions only
need to use the pointer througout execution of the function, and do not
need to store these pointers. In these cases, it's perfectly safe to use
a raw pointer. Furthermore, the overuse of shared_ptr make life time of
FileDescriptors unpredictable. This is harmful in context of VABC, where
presence of a file descriptor can cause UnmapAllPartitions() to fail.
Therefore, we switch some of the safe usecases to raw pointer.
Test: th
Bug: 216391325
Change-Id: I3d986688e81fcc1d761b7f4926a3c81ac691788a
diff --git a/aosp/dynamic_partition_control_android.cc b/aosp/dynamic_partition_control_android.cc
index 825e0d6..6d33a09 100644
--- a/aosp/dynamic_partition_control_android.cc
+++ b/aosp/dynamic_partition_control_android.cc
@@ -1420,7 +1420,7 @@
return snapshot_->OpenSnapshotWriter(params, std::move(source_path));
} // namespace chromeos_update_engine
-FileDescriptorPtr DynamicPartitionControlAndroid::OpenCowFd(
+std::unique_ptr<FileDescriptor> DynamicPartitionControlAndroid::OpenCowFd(
const std::string& unsuffixed_partition_name,
const std::optional<std::string>& source_path,
bool is_append) {
@@ -1438,7 +1438,7 @@
LOG(ERROR) << "ICowWriter::OpenReader() failed.";
return nullptr;
}
- return std::make_shared<CowWriterFileDescriptor>(std::move(cow_writer),
+ return std::make_unique<CowWriterFileDescriptor>(std::move(cow_writer),
std::move(reader));
}
diff --git a/aosp/dynamic_partition_control_android.h b/aosp/dynamic_partition_control_android.h
index d0842a9..cebca07 100644
--- a/aosp/dynamic_partition_control_android.h
+++ b/aosp/dynamic_partition_control_android.h
@@ -104,9 +104,10 @@
const std::string& unsuffixed_partition_name,
const std::optional<std::string>& source_path,
bool is_append) override;
- FileDescriptorPtr OpenCowFd(const std::string& unsuffixed_partition_name,
- const std::optional<std::string>&,
- bool is_append = false) override;
+ std::unique_ptr<FileDescriptor> OpenCowFd(
+ const std::string& unsuffixed_partition_name,
+ const std::optional<std::string>&,
+ bool is_append = false) override;
bool MapAllPartitions() override;
bool UnmapAllPartitions() override;
diff --git a/aosp/mock_dynamic_partition_control_android.h b/aosp/mock_dynamic_partition_control_android.h
index 428b6c7..33ef39c 100644
--- a/aosp/mock_dynamic_partition_control_android.h
+++ b/aosp/mock_dynamic_partition_control_android.h
@@ -94,7 +94,7 @@
const std::optional<std::string>& source_path,
bool is_append),
(override));
- MOCK_METHOD(FileDescriptorPtr,
+ MOCK_METHOD(std::unique_ptr<FileDescriptor>,
OpenCowFd,
(const std::string& unsuffixed_partition_name,
const std::optional<std::string>& source_path,
diff --git a/common/dynamic_partition_control_interface.h b/common/dynamic_partition_control_interface.h
index a5be6e1..e6ebe6a 100644
--- a/common/dynamic_partition_control_interface.h
+++ b/common/dynamic_partition_control_interface.h
@@ -167,7 +167,7 @@
bool is_append = false) = 0;
// Open a general purpose FD capable to reading and writing to COW. Note that
// writes must be block aligned.
- virtual FileDescriptorPtr OpenCowFd(
+ virtual std::unique_ptr<FileDescriptor> OpenCowFd(
const std::string& unsuffixed_partition_name,
const std::optional<std::string>&,
bool is_append = false) = 0;
diff --git a/common/dynamic_partition_control_stub.h b/common/dynamic_partition_control_stub.h
index 515ec7c..5aa4336 100644
--- a/common/dynamic_partition_control_stub.h
+++ b/common/dynamic_partition_control_stub.h
@@ -65,9 +65,10 @@
const std::optional<std::string>&,
bool is_append) override;
- FileDescriptorPtr OpenCowFd(const std::string& unsuffixed_partition_name,
- const std::optional<std::string>&,
- bool is_append = false) override {
+ std::unique_ptr<FileDescriptor> OpenCowFd(
+ const std::string& unsuffixed_partition_name,
+ const std::optional<std::string>&,
+ bool is_append = false) override {
return nullptr;
}
diff --git a/common/mock_dynamic_partition_control.h b/common/mock_dynamic_partition_control.h
index c6b0b2d..f3a446a 100644
--- a/common/mock_dynamic_partition_control.h
+++ b/common/mock_dynamic_partition_control.h
@@ -36,7 +36,7 @@
MOCK_METHOD(FeatureFlag, GetVirtualAbCompressionFeatureFlag, (), (override));
MOCK_METHOD(FeatureFlag, GetVirtualAbFeatureFlag, (), (override));
MOCK_METHOD(bool, FinishUpdate, (bool), (override));
- MOCK_METHOD(FileDescriptorPtr,
+ MOCK_METHOD(std::unique_ptr<FileDescriptor>,
OpenCowFd,
(const std::string& unsuffixed_partition_name,
const std::optional<std::string>& source_path,
diff --git a/common/utils.cc b/common/utils.cc
index aa6c6b3..794b832 100644
--- a/common/utils.cc
+++ b/common/utils.cc
@@ -181,7 +181,7 @@
return true;
}
-bool WriteAll(const FileDescriptorPtr& fd, const void* buf, size_t count) {
+bool WriteAll(FileDescriptor* fd, const void* buf, size_t count) {
const char* c_buf = static_cast<const char*>(buf);
ssize_t bytes_written = 0;
while (bytes_written < static_cast<ssize_t>(count)) {
@@ -218,7 +218,7 @@
return true;
}
-bool ReadAll(const FileDescriptorPtr& fd,
+bool ReadAll(FileDescriptor* fd,
void* buf,
size_t count,
off_t offset,
@@ -239,7 +239,7 @@
return true;
}
-bool PReadAll(const FileDescriptorPtr& fd,
+bool PReadAll(FileDescriptor* fd,
void* buf,
size_t count,
off_t offset,
diff --git a/common/utils.h b/common/utils.h
index a33efb2..0f8da22 100644
--- a/common/utils.h
+++ b/common/utils.h
@@ -68,12 +68,15 @@
bool WriteAll(int fd, const void* buf, size_t count);
bool PWriteAll(int fd, const void* buf, size_t count, off_t offset);
-bool WriteAll(const FileDescriptorPtr& fd, const void* buf, size_t count);
+bool WriteAll(FileDescriptor* fd, const void* buf, size_t count);
+
+constexpr bool WriteAll(const FileDescriptorPtr& fd,
+ const void* buf,
+ size_t count) {
+ return WriteAll(fd.get(), buf, count);
+}
// WriteAll writes data at specified offset, but it modifies file position.
-bool WriteAll(const FileDescriptorPtr& fd,
- const void* buf,
- size_t count,
- off_t off);
+bool WriteAll(FileDescriptorPtr* fd, const void* buf, size_t count, off_t off);
// https://man7.org/linux/man-pages/man2/pread.2.html
// PWriteAll writes data at specified offset, but it DOES NOT modify file
@@ -97,21 +100,38 @@
int fd, void* buf, size_t count, off_t offset, ssize_t* out_bytes_read);
// Reads data at specified offset, this function does change file position.
-bool ReadAll(const FileDescriptorPtr& fd,
+
+bool ReadAll(FileDescriptor* fd,
void* buf,
size_t count,
off_t offset,
ssize_t* out_bytes_read);
+constexpr bool ReadAll(const FileDescriptorPtr& fd,
+ void* buf,
+ size_t count,
+ off_t offset,
+ ssize_t* out_bytes_read) {
+ return ReadAll(fd.get(), buf, count, offset, out_bytes_read);
+}
+
// https://man7.org/linux/man-pages/man2/pread.2.html
// Reads data at specified offset, this function DOES NOT change file position.
// Behavior is similar to linux's pread syscall.
-bool PReadAll(const FileDescriptorPtr& fd,
+bool PReadAll(FileDescriptor* fd,
void* buf,
size_t count,
off_t offset,
ssize_t* out_bytes_read);
+constexpr bool PReadAll(const FileDescriptorPtr& fd,
+ void* buf,
+ size_t count,
+ off_t offset,
+ ssize_t* out_bytes_read) {
+ return PReadAll(fd.get(), buf, count, offset, out_bytes_read);
+}
+
// Opens |path| for reading and appends its entire content to the container
// pointed to by |out_p|. Returns true upon successfully reading all of the
// file's content, false otherwise, in which case the state of the output
diff --git a/payload_consumer/filesystem_verifier_action.cc b/payload_consumer/filesystem_verifier_action.cc
index a8b9269..2770aff 100644
--- a/payload_consumer/filesystem_verifier_action.cc
+++ b/payload_consumer/filesystem_verifier_action.cc
@@ -182,7 +182,7 @@
}
bool FilesystemVerifierAction::InitializeFd(const std::string& part_path) {
- partition_fd_ = FileDescriptorPtr(new EintrSafeFileDescriptor());
+ partition_fd_ = std::make_unique<EintrSafeFileDescriptor>();
const bool write_verity = ShouldWriteVerity();
int flags = write_verity ? O_RDWR : O_RDONLY;
if (!utils::SetBlockDeviceReadOnly(part_path, !write_verity)) {
@@ -197,11 +197,12 @@
}
void FilesystemVerifierAction::WriteVerityAndHashPartition(
- FileDescriptorPtr fd,
const off64_t start_offset,
const off64_t end_offset,
void* buffer,
const size_t buffer_size) {
+ auto fd = partition_fd_.get();
+ TEST_AND_RETURN(fd != nullptr);
if (start_offset >= end_offset) {
LOG_IF(WARNING, start_offset > end_offset)
<< "start_offset is greater than end_offset : " << start_offset << " > "
@@ -219,7 +220,7 @@
return;
}
}
- HashPartition(partition_fd_, 0, partition_size_, buffer, buffer_size);
+ HashPartition(0, partition_size_, buffer, buffer_size);
return;
}
const auto cur_offset = fd->Seek(start_offset, SEEK_SET);
@@ -249,18 +250,18 @@
FROM_HERE,
base::BindOnce(&FilesystemVerifierAction::WriteVerityAndHashPartition,
base::Unretained(this),
- fd,
start_offset + bytes_read,
end_offset,
buffer,
buffer_size)));
}
-void FilesystemVerifierAction::HashPartition(FileDescriptorPtr fd,
- const off64_t start_offset,
+void FilesystemVerifierAction::HashPartition(const off64_t start_offset,
const off64_t end_offset,
void* buffer,
const size_t buffer_size) {
+ auto fd = partition_fd_.get();
+ TEST_AND_RETURN(fd != nullptr);
if (start_offset >= end_offset) {
LOG_IF(WARNING, start_offset > end_offset)
<< "start_offset is greater than end_offset : " << start_offset << " > "
@@ -295,7 +296,6 @@
FROM_HERE,
base::BindOnce(&FilesystemVerifierAction::HashPartition,
base::Unretained(this),
- fd,
start_offset + bytes_read,
end_offset,
buffer,
@@ -361,6 +361,7 @@
CHECK_LE(partition.hash_tree_offset, partition.fec_offset)
<< " Hash tree is expected to come before FEC data";
}
+ CHECK_NE(partition_fd_, nullptr);
if (partition.hash_tree_offset != 0) {
filesystem_data_end_ = partition.hash_tree_offset;
} else if (partition.fec_offset != 0) {
@@ -374,11 +375,10 @@
return;
}
WriteVerityAndHashPartition(
- partition_fd_, 0, filesystem_data_end_, buffer_.data(), buffer_.size());
+ 0, filesystem_data_end_, buffer_.data(), buffer_.size());
} else {
LOG(INFO) << "Verity writes disabled on partition " << partition.name;
- HashPartition(
- partition_fd_, 0, partition_size_, buffer_.data(), buffer_.size());
+ HashPartition(0, partition_size_, buffer_.data(), buffer_.size());
}
}
@@ -430,7 +430,7 @@
Cleanup(ErrorCode::kError);
return;
}
- InstallPlan::Partition& partition =
+ const InstallPlan::Partition& partition =
install_plan_.partitions[partition_index_];
LOG(INFO) << "Hash of " << partition.name << ": "
<< HexEncode(hasher_->raw_hash());
@@ -492,7 +492,6 @@
return;
}
// Start hashing the next partition, if any.
- hasher_.reset();
buffer_.clear();
if (partition_fd_) {
partition_fd_->Close();
diff --git a/payload_consumer/filesystem_verifier_action.h b/payload_consumer/filesystem_verifier_action.h
index 850abda..edc8e53 100644
--- a/payload_consumer/filesystem_verifier_action.h
+++ b/payload_consumer/filesystem_verifier_action.h
@@ -86,13 +86,11 @@
private:
friend class FilesystemVerifierActionTestDelegate;
- void WriteVerityAndHashPartition(FileDescriptorPtr fd,
- const off64_t start_offset,
+ void WriteVerityAndHashPartition(const off64_t start_offset,
const off64_t end_offset,
void* buffer,
const size_t buffer_size);
- void HashPartition(FileDescriptorPtr fd,
- const off64_t start_offset,
+ void HashPartition(const off64_t start_offset,
const off64_t end_offset,
void* buffer,
const size_t buffer_size);
@@ -138,7 +136,7 @@
// If not null, the FileDescriptor used to read from the device.
// verity writer might attempt to write to this fd, if verity is enabled.
- FileDescriptorPtr partition_fd_;
+ std::unique_ptr<FileDescriptor> partition_fd_;
// Buffer for storing data we read.
brillo::Blob buffer_;
diff --git a/payload_consumer/filesystem_verifier_action_unittest.cc b/payload_consumer/filesystem_verifier_action_unittest.cc
index f2f2954..533292a 100644
--- a/payload_consumer/filesystem_verifier_action_unittest.cc
+++ b/payload_consumer/filesystem_verifier_action_unittest.cc
@@ -170,7 +170,7 @@
bytes_to_read -= bytes_read;
offset += bytes_read;
}
- ASSERT_TRUE(verity_writer.Finalize(fd, fd));
+ ASSERT_TRUE(verity_writer.Finalize(fd.get(), fd.get()));
ASSERT_TRUE(fd->IsOpen());
ASSERT_TRUE(HashCalculator::RawHashOfFile(target_part_.path(),
&partition->target_hash));
@@ -565,7 +565,7 @@
EnableVABC(&dynamic_control, part.name);
auto open_cow = [part]() {
- auto cow_fd = std::make_shared<EintrSafeFileDescriptor>();
+ auto cow_fd = std::make_unique<EintrSafeFileDescriptor>();
EXPECT_TRUE(cow_fd->Open(part.readonly_target_path.c_str(), O_RDWR))
<< "Failed to open part " << part.readonly_target_path
<< strerror(errno);
@@ -618,14 +618,14 @@
if (enable_verity) {
std::vector<unsigned char> actual_fec(fec_size);
ssize_t bytes_read = 0;
- ASSERT_TRUE(utils::PReadAll(cow_fd,
+ ASSERT_TRUE(utils::PReadAll(cow_fd.get(),
actual_fec.data(),
actual_fec.size(),
fec_start_offset,
&bytes_read));
ASSERT_EQ(actual_fec, fec_data_);
std::vector<unsigned char> actual_hash_tree(hash_tree_size);
- ASSERT_TRUE(utils::PReadAll(cow_fd,
+ ASSERT_TRUE(utils::PReadAll(cow_fd.get(),
actual_hash_tree.data(),
actual_hash_tree.size(),
HASH_TREE_START_OFFSET,
diff --git a/payload_consumer/verity_writer_android.cc b/payload_consumer/verity_writer_android.cc
index e2fab7d..b233b58 100644
--- a/payload_consumer/verity_writer_android.cc
+++ b/payload_consumer/verity_writer_android.cc
@@ -104,8 +104,8 @@
return true;
}
-bool VerityWriterAndroid::Finalize(FileDescriptorPtr read_fd,
- FileDescriptorPtr write_fd) {
+bool VerityWriterAndroid::Finalize(FileDescriptor* read_fd,
+ FileDescriptor* write_fd) {
const auto hash_tree_data_end =
partition_->hash_tree_data_offset + partition_->hash_tree_data_size;
if (total_offset_ < hash_tree_data_end) {
@@ -142,8 +142,8 @@
return true;
}
-bool VerityWriterAndroid::EncodeFEC(FileDescriptorPtr read_fd,
- FileDescriptorPtr write_fd,
+bool VerityWriterAndroid::EncodeFEC(FileDescriptor* read_fd,
+ FileDescriptor* write_fd,
uint64_t data_offset,
uint64_t data_size,
uint64_t fec_offset,
@@ -162,10 +162,6 @@
init_rs_char(FEC_PARAMS(fec_roots)), &free_rs_char);
TEST_AND_RETURN_FALSE(rs_char != nullptr);
- // Cache at most 1MB of fec data, in VABC, we need to re-open fd if we
- // perform a read() operation after write(). So reduce the number of writes
- // can save unnecessary re-opens.
- write_fd = std::make_shared<CachedFileDescriptor>(write_fd, 1 * (1 << 20));
for (size_t i = 0; i < rounds; i++) {
// Encodes |block_size| number of rs blocks each round so that we can read
// one block each time instead of 1 byte to increase random read
@@ -229,11 +225,10 @@
uint32_t fec_roots,
uint32_t block_size,
bool verify_mode) {
- FileDescriptorPtr fd(new EintrSafeFileDescriptor());
- TEST_AND_RETURN_FALSE(
- fd->Open(path.c_str(), verify_mode ? O_RDONLY : O_RDWR));
- return EncodeFEC(fd,
- fd,
+ EintrSafeFileDescriptor fd;
+ TEST_AND_RETURN_FALSE(fd.Open(path.c_str(), verify_mode ? O_RDONLY : O_RDWR));
+ return EncodeFEC(&fd,
+ &fd,
data_offset,
data_size,
fec_offset,
diff --git a/payload_consumer/verity_writer_android.h b/payload_consumer/verity_writer_android.h
index 8339528..a6a4920 100644
--- a/payload_consumer/verity_writer_android.h
+++ b/payload_consumer/verity_writer_android.h
@@ -34,7 +34,7 @@
bool Init(const InstallPlan::Partition& partition);
bool Update(uint64_t offset, const uint8_t* buffer, size_t size) override;
- bool Finalize(FileDescriptorPtr read_fd, FileDescriptorPtr write_fd) override;
+ bool Finalize(FileDescriptor* read_fd, FileDescriptor* write_fd) override;
// Read [data_offset : data_offset + data_size) from |path| and encode FEC
// data, if |verify_mode|, then compare the encoded FEC with the one in
@@ -42,8 +42,8 @@
// in each Update() like hash tree, because for every rs block, its data are
// spreaded across entire |data_size|, unless we can cache all data in
// memory, we have to re-read them from disk.
- static bool EncodeFEC(FileDescriptorPtr read_fd,
- FileDescriptorPtr write_fd,
+ static bool EncodeFEC(FileDescriptor* read_fd,
+ FileDescriptor* write_fd,
uint64_t data_offset,
uint64_t data_size,
uint64_t fec_offset,
diff --git a/payload_consumer/verity_writer_android_unittest.cc b/payload_consumer/verity_writer_android_unittest.cc
index 75da0ae..693bcda 100644
--- a/payload_consumer/verity_writer_android_unittest.cc
+++ b/payload_consumer/verity_writer_android_unittest.cc
@@ -54,7 +54,8 @@
ASSERT_TRUE(verity_writer_.Init(partition_));
ASSERT_TRUE(verity_writer_.Update(0, part_data.data(), 4096));
ASSERT_TRUE(verity_writer_.Update(4096, part_data.data() + 4096, 4096));
- ASSERT_TRUE(verity_writer_.Finalize(partition_fd_, partition_fd_));
+ ASSERT_TRUE(
+ verity_writer_.Finalize(partition_fd_.get(), partition_fd_.get()));
brillo::Blob actual_part;
utils::ReadFile(partition_.target_path, &actual_part);
// dd if=/dev/zero bs=4096 count=1 2>/dev/null | sha1sum | xxd -r -p |
@@ -102,7 +103,8 @@
ASSERT_TRUE(verity_writer_.Init(partition_));
ASSERT_TRUE(verity_writer_.Update(0, part_data.data(), 4096));
ASSERT_TRUE(verity_writer_.Update(4096, part_data.data() + 4096, 4096));
- ASSERT_TRUE(verity_writer_.Finalize(partition_fd_, partition_fd_));
+ ASSERT_TRUE(
+ verity_writer_.Finalize(partition_fd_.get(), partition_fd_.get()));
brillo::Blob actual_part;
utils::ReadFile(partition_.target_path, &actual_part);
// dd if=/dev/zero bs=4096 count=1 2>/dev/null | sha256sum | xxd -r -p |
@@ -127,7 +129,8 @@
ASSERT_TRUE(verity_writer_.Update(4096, part_data.data() + 4096, 4096));
ASSERT_TRUE(verity_writer_.Update(
8192, part_data.data() + 8192, partition_.hash_tree_data_offset));
- ASSERT_TRUE(verity_writer_.Finalize(partition_fd_, partition_fd_));
+ ASSERT_TRUE(
+ verity_writer_.Finalize(partition_fd_.get(), partition_fd_.get()));
brillo::Blob actual_part;
utils::ReadFile(partition_.target_path, &actual_part);
// dd if=/dev/zero bs=4096 count=1 2>/dev/null | sha256sum | xxd -r -p |
@@ -150,7 +153,8 @@
test_utils::WriteFileVector(partition_.target_path, part_data);
ASSERT_TRUE(verity_writer_.Init(partition_));
ASSERT_TRUE(verity_writer_.Update(0, part_data.data(), part_data.size()));
- ASSERT_TRUE(verity_writer_.Finalize(partition_fd_, partition_fd_));
+ ASSERT_TRUE(
+ verity_writer_.Finalize(partition_fd_.get(), partition_fd_.get()));
brillo::Blob actual_part;
utils::ReadFile(partition_.target_path, &actual_part);
// Write FEC data.
diff --git a/payload_consumer/verity_writer_interface.h b/payload_consumer/verity_writer_interface.h
index 37ed605..432ede7 100644
--- a/payload_consumer/verity_writer_interface.h
+++ b/payload_consumer/verity_writer_interface.h
@@ -39,8 +39,7 @@
virtual bool Update(uint64_t offset, const uint8_t* buffer, size_t size) = 0;
// Write hash tree && FEC data to underlying fd, if they are present
- virtual bool Finalize(FileDescriptorPtr read_fd,
- FileDescriptorPtr write_fd) = 0;
+ virtual bool Finalize(FileDescriptor* read_fd, FileDescriptor* write_fd) = 0;
protected:
VerityWriterInterface() = default;