summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
author Richard Neill <richard.neill@arm.com> 2024-03-05 16:18:14 +0000
committer Jiakai Zhang <jiakaiz@google.com> 2024-03-25 15:01:42 +0000
commit00fbc4047bd6a1984c333b93fc39e73e61a2521a (patch)
tree1ea128bd50230a40a75363a590a6af1d07301d2c
parentc5e25a99840aeb9b60e914287573bdef16fcb292 (diff)
Add and use FdFile::Rename on Android V+
Add the ability to change a file's path via the Rename function, and use it when installing staging files during odrefresh. As the moved file requires its SELinux label updated via `selinux_android_restorecon` which odrefresh requires an updated SELinux policy to use, only perform the rename on Android V+ and otherwise fallback to the existing copy and delete approach. On linux, the rename syscall requires the source and destination to be on the same mounted filesystem. As this approach does not modify the file's contents, this has the added benefit of ensuring the sparsity of the resulting file in the destination path is preserved, thereby saving disk space on systems that support sparse files. As rename applies changes to parent directory metadata, FdFile now supports flushing metadata in addition to data (via fsync), as well as allowing read-only file descriptors to be flushed (required for directories). Bug: 323906594 Tested on oriole and shiba (ext4 and f2fs): Test: odrefresh --force-compile Test: art/tools/run-gtests.sh Change-Id: I1d017552fbffcc728cac7a55a14d4d17d1770cb8
-rw-r--r--libartbase/base/unix_file/fd_file.cc90
-rw-r--r--libartbase/base/unix_file/fd_file.h23
-rw-r--r--libartbase/base/unix_file/fd_file_test.cc69
-rw-r--r--odrefresh/odrefresh.cc92
4 files changed, 220 insertions, 54 deletions
diff --git a/libartbase/base/unix_file/fd_file.cc b/libartbase/base/unix_file/fd_file.cc
index c5307e249b..f5f20214f5 100644
--- a/libartbase/base/unix_file/fd_file.cc
+++ b/libartbase/base/unix_file/fd_file.cc
@@ -17,6 +17,7 @@
#include "fd_file.h"
#include <errno.h>
+#include <stdio.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>
@@ -30,6 +31,7 @@
#endif
#include <limits>
+#include <vector>
#include <android-base/file.h>
#include <android-base/logging.h>
@@ -341,11 +343,16 @@ int FdFile::Close() {
return 0;
}
-int FdFile::Flush() {
- DCHECK(!read_only_mode_);
+int FdFile::Flush(bool flush_metadata) {
+ DCHECK(flush_metadata || !read_only_mode_);
#ifdef __linux__
- int rc = TEMP_FAILURE_RETRY(fdatasync(fd_));
+ int rc;
+ if (flush_metadata) {
+ rc = TEMP_FAILURE_RETRY(fsync(fd_));
+ } else {
+ rc = TEMP_FAILURE_RETRY(fdatasync(fd_));
+ }
#else
int rc = TEMP_FAILURE_RETRY(fsync(fd_));
#endif
@@ -470,6 +477,56 @@ bool FdFile::WriteFully(const void* buffer, size_t byte_count) {
return WriteFullyGeneric<false>(buffer, byte_count, 0u);
}
+bool FdFile::Rename(const std::string& new_path) {
+ if (kCheckSafeUsage) {
+ // Filesystems that use delayed allocation (e.g., ext4) may journal a rename before a data
+ // update is written to disk. Therefore on system crash, the data update may not persist.
+ // Guard against this by ensuring the file has been flushed prior to rename.
+ if (guard_state_ < GuardState::kFlushed) {
+ LOG(ERROR) << "File " << file_path_ << " has not been flushed before renaming.";
+ }
+ DCHECK_GE(guard_state_, GuardState::kFlushed);
+ }
+
+ if (!FilePathMatchesFd()) {
+ LOG(ERROR) << "Failed rename because the file descriptor is not backed by the expected file "
+ << "path: " << file_path_;
+ return false;
+ }
+
+ std::string old_path = file_path_;
+ int rc = std::rename(old_path.c_str(), new_path.c_str());
+ if (rc != 0) {
+ LOG(ERROR) << "Rename from '" << old_path << "' to '" << new_path << "' failed.";
+ return false;
+ }
+ file_path_ = new_path;
+
+ // Rename modifies the directory entries mapped within the parent directory file descriptor(s),
+ // rather than the file, so flushing the file will not persist the change to disk. Therefore, we
+ // flush the parent directory file descriptor(s).
+ std::string old_dir = android::base::Dirname(old_path);
+ std::string new_dir = android::base::Dirname(new_path);
+ std::vector<std::string> sync_dirs = {new_dir};
+ if (new_dir != old_dir) {
+ sync_dirs.emplace_back(old_dir);
+ }
+ for (auto& dirname : sync_dirs) {
+ FdFile dir = FdFile(dirname, O_RDONLY, /*check_usage=*/false);
+ rc = dir.Flush(/*flush_metadata=*/true);
+ if (rc != 0) {
+ LOG(ERROR) << "Flushing directory '" << dirname << "' during rename failed.";
+ return false;
+ }
+ rc = dir.Close();
+ if (rc != 0) {
+ LOG(ERROR) << "Closing directory '" << dirname << "' during rename failed.";
+ return false;
+ }
+ }
+ return true;
+}
+
bool FdFile::Copy(FdFile* input_file, int64_t offset, int64_t size) {
DCHECK(!read_only_mode_);
off_t off = static_cast<off_t>(offset);
@@ -517,26 +574,27 @@ bool FdFile::Copy(FdFile* input_file, int64_t offset, int64_t size) {
return true;
}
-bool FdFile::Unlink() {
+bool FdFile::FilePathMatchesFd() {
if (file_path_.empty()) {
return false;
}
-
- // Try to figure out whether this file is still referring to the one on disk.
+ // Try to figure out whether file_path_ is still referring to the one on disk.
bool is_current = false;
- {
- struct stat this_stat, current_stat;
- int cur_fd = TEMP_FAILURE_RETRY(open(file_path_.c_str(), O_RDONLY | O_CLOEXEC));
- if (cur_fd > 0) {
- // File still exists.
- if (fstat(fd_, &this_stat) == 0 && fstat(cur_fd, &current_stat) == 0) {
- is_current = (this_stat.st_dev == current_stat.st_dev) &&
- (this_stat.st_ino == current_stat.st_ino);
- }
- close(cur_fd);
+ struct stat this_stat, current_stat;
+ int cur_fd = TEMP_FAILURE_RETRY(open(file_path_.c_str(), O_RDONLY | O_CLOEXEC));
+ if (cur_fd > 0) {
+ // File still exists.
+ if (fstat(fd_, &this_stat) == 0 && fstat(cur_fd, &current_stat) == 0) {
+ is_current = (this_stat.st_dev == current_stat.st_dev) &&
+ (this_stat.st_ino == current_stat.st_ino);
}
+ close(cur_fd);
}
+ return is_current;
+}
+bool FdFile::Unlink() {
+ bool is_current = FilePathMatchesFd();
if (is_current) {
unlink(file_path_.c_str());
}
diff --git a/libartbase/base/unix_file/fd_file.h b/libartbase/base/unix_file/fd_file.h
index 39471010df..5e09373b59 100644
--- a/libartbase/base/unix_file/fd_file.h
+++ b/libartbase/base/unix_file/fd_file.h
@@ -73,16 +73,14 @@ class FdFile : public RandomAccessFile {
int64_t GetLength() const override;
int64_t Write(const char* buf, int64_t byte_count, int64_t offset) override WARN_UNUSED;
- int Flush() override WARN_UNUSED;
+ int Flush() override WARN_UNUSED { return Flush(/*flush_metadata=*/false); }
// Short for SetLength(0); Flush(); Close();
// If the file was opened with a path name and unlink = true, also calls Unlink() on the path.
// Note that it is the the caller's responsibility to avoid races.
bool Erase(bool unlink = false);
- // Call unlink() if the file was opened with a path, and if open() with the name shows that
- // the file descriptor of this file is still up-to-date. This is still racy, though, and it
- // is up to the caller to ensure correctness in a multi-process setup.
+ // Call unlink(), though only if FilePathMatchesFd() returns true.
bool Unlink();
// Try to Flush(), then try to Close(); If either fails, call Erase().
@@ -110,6 +108,15 @@ class FdFile : public RandomAccessFile {
bool WriteFully(const void* buffer, size_t byte_count) WARN_UNUSED;
bool PwriteFully(const void* buffer, size_t byte_count, size_t offset) WARN_UNUSED;
+ // Change the file path, though only if FilePathMatchesFd() returns true.
+ //
+ // If a file at new_path already exists, it will be replaced.
+ // On Linux, the rename syscall will fail unless the source and destination are on the same
+ // mounted filesystem.
+ // This function is not expected to modify the file data itself, instead it modifies the inodes of
+ // the source and destination directories, and therefore the function flushes those file
+ // descriptors following the rename.
+ bool Rename(const std::string& new_path);
// Copy data from another file.
bool Copy(FdFile* input_file, int64_t offset, int64_t size);
// Clears the file content and resets the file offset to 0.
@@ -165,6 +172,14 @@ class FdFile : public RandomAccessFile {
template <bool kUseOffset>
bool WriteFullyGeneric(const void* buffer, size_t byte_count, size_t offset);
+ int Flush(bool flush_metadata) WARN_UNUSED;
+
+ // The file path we hold for the file descriptor may be invalid, or may not even exist (e.g. if
+ // the FdFile wasn't initialised with a path). This helper function checks if calling open() on
+ // the file path (if it is set) returns the expected up-to-date file descriptor. This is still
+ // racy, though, and it is up to the caller to ensure correctness in a multi-process setup.
+ bool FilePathMatchesFd();
+
void Destroy(); // For ~FdFile and operator=(&&).
int fd_ = kInvalidFd;
diff --git a/libartbase/base/unix_file/fd_file_test.cc b/libartbase/base/unix_file/fd_file_test.cc
index 92f8308b8c..433e3084b0 100644
--- a/libartbase/base/unix_file/fd_file_test.cc
+++ b/libartbase/base/unix_file/fd_file_test.cc
@@ -158,6 +158,73 @@ TEST_F(FdFileTest, ReadWriteFullyWithOffset) {
ASSERT_EQ(file.Close(), 0);
}
+TEST_F(FdFileTest, Rename) {
+ art::ScratchFile src_tmp;
+ FdFile* src = src_tmp.GetFile();
+ ASSERT_TRUE(src->IsOpened());
+
+ // To test that rename preserves sparsity (on systems that support file sparsity), create a source
+ // file with 3 chunks: 2 data chunks separated by an empty 'hole' chunk.
+ constexpr size_t kChunkSize = 64 * art::KB;
+ std::vector<int8_t> data_buffer(kChunkSize, 1);
+ std::vector<int8_t> zero_buffer(kChunkSize, 0);
+ ASSERT_TRUE(src->WriteFully(data_buffer.data(), kChunkSize));
+ ASSERT_GT(lseek(src->Fd(), kChunkSize, SEEK_CUR), 0);
+ ASSERT_TRUE(src->WriteFully(data_buffer.data(), kChunkSize));
+ ASSERT_EQ(src->Flush(), 0);
+
+ size_t src_offset = lseek(src->Fd(), 0, SEEK_CUR);
+ size_t source_length = 3 * kChunkSize;
+
+ // Confirm the source file's length is as expected, to be used later.
+ EXPECT_EQ(src->GetLength(), source_length);
+
+ struct stat src_stat;
+ ASSERT_EQ(fstat(src->Fd(), &src_stat), 0);
+
+ // Move the file via a rename.
+ art::ScratchFile dest_tmp;
+ std::string new_filename = dest_tmp.GetFilename();
+ std::string old_filename = src->GetPath();
+ ASSERT_TRUE(src->Rename(new_filename));
+
+ // Confirm the FdFile path has correctly updated.
+ EXPECT_EQ(src->GetPath(), new_filename);
+ // Check the offset of the moved file has not been modified.
+ EXPECT_EQ(lseek(src->Fd(), 0, SEEK_CUR), src_offset);
+
+ ASSERT_EQ(src->Close(), 0);
+
+ // Test that the file no longer exists in the old location, and there is a file at the new
+ // location with the expected length.
+ EXPECT_FALSE(art::OS::FileExists(old_filename.c_str()));
+ FdFile dest(new_filename, O_RDONLY, /*check_usage=*/false);
+ ASSERT_TRUE(dest.IsOpened());
+ EXPECT_EQ(dest.GetLength(), source_length);
+
+ // Confirm the file at the new location has the same number of allocated data blocks as the source
+ // file. If the source file was a sparse file, this confirms that the sparsity was preserved
+ // by the move.
+ struct stat dest_stat;
+ ASSERT_EQ(fstat(dest.Fd(), &dest_stat), 0);
+ EXPECT_EQ(dest_stat.st_blocks, src_stat.st_blocks);
+
+ // And it is exactly the same file in the new location, with the same contents.
+ EXPECT_EQ(dest_stat.st_dev, src_stat.st_dev);
+ EXPECT_EQ(dest_stat.st_ino, src_stat.st_ino);
+
+ ASSERT_EQ(lseek(dest.Fd(), 0, SEEK_CUR), 0);
+ std::vector<int8_t> check_buffer(kChunkSize);
+ ASSERT_TRUE(dest.ReadFully(check_buffer.data(), kChunkSize));
+ EXPECT_EQ(0, memcmp(check_buffer.data(), data_buffer.data(), kChunkSize));
+ ASSERT_TRUE(dest.ReadFully(check_buffer.data(), kChunkSize));
+ EXPECT_EQ(0, memcmp(check_buffer.data(), zero_buffer.data(), kChunkSize));
+ ASSERT_TRUE(dest.ReadFully(check_buffer.data(), kChunkSize));
+ EXPECT_EQ(0, memcmp(check_buffer.data(), data_buffer.data(), kChunkSize));
+
+ EXPECT_EQ(dest.Close(), 0);
+}
+
TEST_F(FdFileTest, Copy) {
art::ScratchFile src_tmp;
FdFile src(src_tmp.GetFilename(), O_RDWR, false);
@@ -170,7 +237,7 @@ TEST_F(FdFileTest, Copy) {
ASSERT_EQ(static_cast<int64_t>(sizeof(src_data)), src.GetLength());
art::ScratchFile dest_tmp;
- FdFile dest(src_tmp.GetFilename(), O_RDWR, false);
+ FdFile dest(dest_tmp.GetFilename(), O_RDWR, false);
ASSERT_GE(dest.Fd(), 0);
ASSERT_TRUE(dest.IsOpened());
diff --git a/odrefresh/odrefresh.cc b/odrefresh/odrefresh.cc
index c87629df77..9a3f9f67cb 100644
--- a/odrefresh/odrefresh.cc
+++ b/odrefresh/odrefresh.cc
@@ -85,6 +85,7 @@
#include "odr_fs_utils.h"
#include "odr_metrics.h"
#include "odrefresh/odrefresh.h"
+#include "selinux/android.h"
#include "selinux/selinux.h"
#include "tools/cmdline_builder.h"
@@ -152,43 +153,68 @@ bool MoveOrEraseFiles(const std::vector<std::unique_ptr<File>>& files,
std::string output_file_path = ART_FORMAT("{}/{}", output_directory_path, file_basename);
std::string input_file_path = file->GetPath();
- output_files.emplace_back(OS::CreateEmptyFileWriteOnly(output_file_path.c_str()));
- if (output_files.back() == nullptr) {
- PLOG(ERROR) << "Failed to open " << QuotePath(output_file_path);
- output_files.pop_back();
- EraseFiles(output_files);
- EraseFiles(files);
- return false;
- }
+ if (IsAtLeastV()) {
+ // Simply rename the existing file. Requires at least V as odrefresh does not have
+ // `selinux_android_restorecon` permissions on U and lower.
+ if (!file->Rename(output_file_path)) {
+ PLOG(ERROR) << "Failed to rename " << QuotePath(input_file_path) << " to "
+ << QuotePath(output_file_path);
+ EraseFiles(files);
+ return false;
+ }
- if (fchmod(output_files.back()->Fd(), kFileMode) != 0) {
- PLOG(ERROR) << "Could not set file mode on " << QuotePath(output_file_path);
- EraseFiles(output_files);
- EraseFiles(files);
- return false;
- }
+ if (file->FlushCloseOrErase() != 0) {
+ PLOG(ERROR) << "Failed to flush and close file " << QuotePath(output_file_path);
+ EraseFiles(files);
+ return false;
+ }
- size_t file_bytes = file->GetLength();
- if (!output_files.back()->Copy(file.get(), /*offset=*/0, file_bytes)) {
- PLOG(ERROR) << "Failed to copy " << QuotePath(file->GetPath()) << " to "
- << QuotePath(output_file_path);
- EraseFiles(output_files);
- EraseFiles(files);
- return false;
- }
+ if (selinux_android_restorecon(output_file_path.c_str(), 0) < 0) {
+ LOG(ERROR) << "Failed to set security context for file " << QuotePath(output_file_path);
+ EraseFiles(files);
+ return false;
+ }
+ } else {
+ // Create a new file in the output directory, copy the input file's data across, then delete
+ // the input file.
+ output_files.emplace_back(OS::CreateEmptyFileWriteOnly(output_file_path.c_str()));
+ if (output_files.back() == nullptr) {
+ PLOG(ERROR) << "Failed to open " << QuotePath(output_file_path);
+ output_files.pop_back();
+ EraseFiles(output_files);
+ EraseFiles(files);
+ return false;
+ }
- if (!file->Erase(/*unlink=*/true)) {
- PLOG(ERROR) << "Failed to erase " << QuotePath(file->GetPath());
- EraseFiles(output_files);
- EraseFiles(files);
- return false;
- }
+ if (fchmod(output_files.back()->Fd(), kFileMode) != 0) {
+ PLOG(ERROR) << "Could not set file mode on " << QuotePath(output_file_path);
+ EraseFiles(output_files);
+ EraseFiles(files);
+ return false;
+ }
- if (output_files.back()->FlushCloseOrErase() != 0) {
- PLOG(ERROR) << "Failed to flush and close file " << QuotePath(output_file_path);
- EraseFiles(output_files);
- EraseFiles(files);
- return false;
+ size_t file_bytes = file->GetLength();
+ if (!output_files.back()->Copy(file.get(), /*offset=*/0, file_bytes)) {
+ PLOG(ERROR) << "Failed to copy " << QuotePath(file->GetPath()) << " to "
+ << QuotePath(output_file_path);
+ EraseFiles(output_files);
+ EraseFiles(files);
+ return false;
+ }
+
+ if (!file->Erase(/*unlink=*/true)) {
+ PLOG(ERROR) << "Failed to erase " << QuotePath(file->GetPath());
+ EraseFiles(output_files);
+ EraseFiles(files);
+ return false;
+ }
+
+ if (output_files.back()->FlushCloseOrErase() != 0) {
+ PLOG(ERROR) << "Failed to flush and close file " << QuotePath(output_file_path);
+ EraseFiles(output_files);
+ EraseFiles(files);
+ return false;
+ }
}
}
return true;