ART: Track Flush & Close in FdFile
Implement a check that aborts when a file hasn't been explicitly
flushed and closed when it is destructed.
Add WARN_UNUSED to FdFile methods.
Update dex2oat, patchoat, scoped_flock and some gtests to pass with
this.
(cherry picked from commit 9433ec60b325b708b9fa87e699ab4a6565741494)
Change-Id: I9ab03b1653e69f44cc98946dc89d764c3e045dd4
diff --git a/runtime/base/scoped_flock.cc b/runtime/base/scoped_flock.cc
index bf091d0..0e93eee 100644
--- a/runtime/base/scoped_flock.cc
+++ b/runtime/base/scoped_flock.cc
@@ -27,6 +27,9 @@
bool ScopedFlock::Init(const char* filename, std::string* error_msg) {
while (true) {
+ if (file_.get() != nullptr) {
+ UNUSED(file_->FlushCloseOrErase()); // Ignore result.
+ }
file_.reset(OS::OpenFileWithFlags(filename, O_CREAT | O_RDWR));
if (file_.get() == NULL) {
*error_msg = StringPrintf("Failed to open file '%s': %s", filename, strerror(errno));
@@ -59,7 +62,7 @@
}
bool ScopedFlock::Init(File* file, std::string* error_msg) {
- file_.reset(new File(dup(file->Fd())));
+ file_.reset(new File(dup(file->Fd()), true));
if (file_->Fd() == -1) {
file_.reset();
*error_msg = StringPrintf("Failed to duplicate open file '%s': %s",
@@ -89,6 +92,9 @@
if (file_.get() != NULL) {
int flock_result = TEMP_FAILURE_RETRY(flock(file_->Fd(), LOCK_UN));
CHECK_EQ(0, flock_result);
+ if (file_->FlushCloseOrErase() != 0) {
+ PLOG(WARNING) << "Could not close scoped file lock file.";
+ }
}
}
diff --git a/runtime/base/unix_file/fd_file.cc b/runtime/base/unix_file/fd_file.cc
index f29a7ec..6e5e7a1 100644
--- a/runtime/base/unix_file/fd_file.cc
+++ b/runtime/base/unix_file/fd_file.cc
@@ -14,28 +14,68 @@
* limitations under the License.
*/
-#include "base/logging.h"
#include "base/unix_file/fd_file.h"
+
#include <errno.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>
+#include "base/logging.h"
+
namespace unix_file {
-FdFile::FdFile() : fd_(-1), auto_close_(true) {
+FdFile::FdFile() : guard_state_(GuardState::kClosed), fd_(-1), auto_close_(true) {
}
-FdFile::FdFile(int fd) : fd_(fd), auto_close_(true) {
+FdFile::FdFile(int fd, bool check_usage)
+ : guard_state_(check_usage ? GuardState::kBase : GuardState::kNoCheck),
+ fd_(fd), auto_close_(true) {
}
-FdFile::FdFile(int fd, const std::string& path) : fd_(fd), file_path_(path), auto_close_(true) {
+FdFile::FdFile(int fd, const std::string& path, bool check_usage)
+ : guard_state_(check_usage ? GuardState::kBase : GuardState::kNoCheck),
+ fd_(fd), file_path_(path), auto_close_(true) {
CHECK_NE(0U, path.size());
}
FdFile::~FdFile() {
+ if (kCheckSafeUsage && (guard_state_ < GuardState::kNoCheck)) {
+ if (guard_state_ < GuardState::kFlushed) {
+ LOG(::art::ERROR) << "File " << file_path_ << " wasn't explicitly flushed before destruction.";
+ }
+ if (guard_state_ < GuardState::kClosed) {
+ LOG(::art::ERROR) << "File " << file_path_ << " wasn't explicitly closed before destruction.";
+ }
+ CHECK_GE(guard_state_, GuardState::kClosed);
+ }
if (auto_close_ && fd_ != -1) {
- Close();
+ if (Close() != 0) {
+ PLOG(::art::WARNING) << "Failed to close file " << file_path_;
+ }
+ }
+}
+
+void FdFile::moveTo(GuardState target, GuardState warn_threshold, const char* warning) {
+ if (kCheckSafeUsage) {
+ if (guard_state_ < GuardState::kNoCheck) {
+ if (warn_threshold < GuardState::kNoCheck && guard_state_ >= warn_threshold) {
+ LOG(::art::ERROR) << warning;
+ }
+ guard_state_ = target;
+ }
+ }
+}
+
+void FdFile::moveUp(GuardState target, const char* warning) {
+ if (kCheckSafeUsage) {
+ if (guard_state_ < GuardState::kNoCheck) {
+ if (guard_state_ < target) {
+ guard_state_ = target;
+ } else if (target < guard_state_) {
+ LOG(::art::ERROR) << warning;
+ }
+ }
}
}
@@ -54,11 +94,28 @@
return false;
}
file_path_ = path;
+ static_assert(O_RDONLY == 0, "Readonly flag has unexpected value.");
+ if (kCheckSafeUsage && (flags & (O_RDWR | O_CREAT | O_WRONLY)) != 0) {
+ // Start in the base state (not flushed, not closed).
+ guard_state_ = GuardState::kBase;
+ } else {
+ // We are not concerned with read-only files. In that case, proper flushing and closing is
+ // not important.
+ guard_state_ = GuardState::kNoCheck;
+ }
return true;
}
int FdFile::Close() {
int result = TEMP_FAILURE_RETRY(close(fd_));
+
+ // Test here, so the file is closed and not leaked.
+ if (kCheckSafeUsage) {
+ CHECK_GE(guard_state_, GuardState::kFlushed) << "File " << file_path_
+ << " has not been flushed before closing.";
+ moveUp(GuardState::kClosed, nullptr);
+ }
+
if (result == -1) {
return -errno;
} else {
@@ -74,6 +131,7 @@
#else
int rc = TEMP_FAILURE_RETRY(fsync(fd_));
#endif
+ moveUp(GuardState::kFlushed, "Flushing closed file.");
return (rc == -1) ? -errno : rc;
}
@@ -92,6 +150,7 @@
#else
int rc = TEMP_FAILURE_RETRY(ftruncate(fd_, new_length));
#endif
+ moveTo(GuardState::kBase, GuardState::kClosed, "Truncating closed file.");
return (rc == -1) ? -errno : rc;
}
@@ -107,6 +166,7 @@
#else
int rc = TEMP_FAILURE_RETRY(pwrite(fd_, buf, byte_count, offset));
#endif
+ moveTo(GuardState::kBase, GuardState::kClosed, "Writing into closed file.");
return (rc == -1) ? -errno : rc;
}
@@ -135,6 +195,7 @@
bool FdFile::WriteFully(const void* buffer, size_t byte_count) {
const char* ptr = static_cast<const char*>(buffer);
+ moveTo(GuardState::kBase, GuardState::kClosed, "Writing into closed file.");
while (byte_count > 0) {
ssize_t bytes_written = TEMP_FAILURE_RETRY(write(fd_, ptr, byte_count));
if (bytes_written == -1) {
@@ -146,4 +207,38 @@
return true;
}
+void FdFile::Erase() {
+ TEMP_FAILURE_RETRY(SetLength(0));
+ TEMP_FAILURE_RETRY(Flush());
+ TEMP_FAILURE_RETRY(Close());
+}
+
+int FdFile::FlushCloseOrErase() {
+ int flush_result = TEMP_FAILURE_RETRY(Flush());
+ if (flush_result != 0) {
+ LOG(::art::ERROR) << "CloseOrErase failed while flushing a file.";
+ Erase();
+ return flush_result;
+ }
+ int close_result = TEMP_FAILURE_RETRY(Close());
+ if (close_result != 0) {
+ LOG(::art::ERROR) << "CloseOrErase failed while closing a file.";
+ Erase();
+ return close_result;
+ }
+ return 0;
+}
+
+int FdFile::FlushClose() {
+ int flush_result = TEMP_FAILURE_RETRY(Flush());
+ if (flush_result != 0) {
+ LOG(::art::ERROR) << "FlushClose failed while flushing a file.";
+ }
+ int close_result = TEMP_FAILURE_RETRY(Close());
+ if (close_result != 0) {
+ LOG(::art::ERROR) << "FlushClose failed while closing a file.";
+ }
+ return (flush_result != 0) ? flush_result : close_result;
+}
+
} // namespace unix_file
diff --git a/runtime/base/unix_file/fd_file.h b/runtime/base/unix_file/fd_file.h
index 01f4ca2..8db2ee4 100644
--- a/runtime/base/unix_file/fd_file.h
+++ b/runtime/base/unix_file/fd_file.h
@@ -24,6 +24,9 @@
namespace unix_file {
+// If true, check whether Flush and Close are called before destruction.
+static constexpr bool kCheckSafeUsage = true;
+
// A RandomAccessFile implementation backed by a file descriptor.
//
// Not thread safe.
@@ -32,8 +35,8 @@
FdFile();
// Creates an FdFile using the given file descriptor. Takes ownership of the
// file descriptor. (Use DisableAutoClose to retain ownership.)
- explicit FdFile(int fd);
- explicit FdFile(int fd, const std::string& path);
+ explicit FdFile(int fd, bool checkUsage);
+ explicit FdFile(int fd, const std::string& path, bool checkUsage);
// Destroys an FdFile, closing the file descriptor if Close hasn't already
// been called. (If you care about the return value of Close, call it
@@ -47,12 +50,21 @@
bool Open(const std::string& file_path, int flags, mode_t mode);
// RandomAccessFile API.
- virtual int Close();
- virtual int64_t Read(char* buf, int64_t byte_count, int64_t offset) const;
- virtual int SetLength(int64_t new_length);
+ virtual int Close() WARN_UNUSED;
+ virtual int64_t Read(char* buf, int64_t byte_count, int64_t offset) const WARN_UNUSED;
+ virtual int SetLength(int64_t new_length) WARN_UNUSED;
virtual int64_t GetLength() const;
- virtual int64_t Write(const char* buf, int64_t byte_count, int64_t offset);
- virtual int Flush();
+ virtual int64_t Write(const char* buf, int64_t byte_count, int64_t offset) WARN_UNUSED;
+ virtual int Flush() WARN_UNUSED;
+
+ // Short for SetLength(0); Flush(); Close();
+ void Erase();
+
+ // Try to Flush(), then try to Close(); If either fails, call Erase().
+ int FlushCloseOrErase() WARN_UNUSED;
+
+ // Try to Flush and Close(). Attempts both, but returns the first error.
+ int FlushClose() WARN_UNUSED;
// Bonus API.
int Fd() const;
@@ -61,8 +73,35 @@
return file_path_;
}
void DisableAutoClose();
- bool ReadFully(void* buffer, size_t byte_count);
- bool WriteFully(const void* buffer, size_t byte_count);
+ bool ReadFully(void* buffer, size_t byte_count) WARN_UNUSED;
+ bool WriteFully(const void* buffer, size_t byte_count) WARN_UNUSED;
+
+ // This enum is public so that we can define the << operator over it.
+ enum class GuardState {
+ kBase, // Base, file has not been flushed or closed.
+ kFlushed, // File has been flushed, but not closed.
+ kClosed, // File has been flushed and closed.
+ kNoCheck // Do not check for the current file instance.
+ };
+
+ protected:
+ // If the guard state indicates checking (!=kNoCheck), go to the target state "target". Print the
+ // given warning if the current state is or exceeds warn_threshold.
+ void moveTo(GuardState target, GuardState warn_threshold, const char* warning);
+
+ // If the guard state indicates checking (<kNoCheck), and is below the target state "target", go
+ // to "target." If the current state is higher (excluding kNoCheck) than the trg state, print the
+ // warning.
+ void moveUp(GuardState target, const char* warning);
+
+ // Forcefully sets the state to the given one. This can overwrite kNoCheck.
+ void resetGuard(GuardState new_state) {
+ if (kCheckSafeUsage) {
+ guard_state_ = new_state;
+ }
+ }
+
+ GuardState guard_state_;
private:
int fd_;
@@ -72,6 +111,8 @@
DISALLOW_COPY_AND_ASSIGN(FdFile);
};
+std::ostream& operator<<(std::ostream& os, const FdFile::GuardState& kind);
+
} // namespace unix_file
#endif // ART_RUNTIME_BASE_UNIX_FILE_FD_FILE_H_
diff --git a/runtime/base/unix_file/fd_file_test.cc b/runtime/base/unix_file/fd_file_test.cc
index 3481f2f..a7e5b96 100644
--- a/runtime/base/unix_file/fd_file_test.cc
+++ b/runtime/base/unix_file/fd_file_test.cc
@@ -24,7 +24,7 @@
class FdFileTest : public RandomAccessFileTest {
protected:
virtual RandomAccessFile* MakeTestFile() {
- return new FdFile(fileno(tmpfile()));
+ return new FdFile(fileno(tmpfile()), false);
}
};
@@ -53,6 +53,7 @@
ASSERT_TRUE(file.Open(good_path, O_CREAT | O_WRONLY));
EXPECT_GE(file.Fd(), 0);
EXPECT_TRUE(file.IsOpened());
+ EXPECT_EQ(0, file.Flush());
EXPECT_EQ(0, file.Close());
EXPECT_EQ(-1, file.Fd());
EXPECT_FALSE(file.IsOpened());
@@ -60,7 +61,7 @@
EXPECT_GE(file.Fd(), 0);
EXPECT_TRUE(file.IsOpened());
- file.Close();
+ ASSERT_EQ(file.Close(), 0);
ASSERT_EQ(unlink(good_path.c_str()), 0);
}
diff --git a/runtime/base/unix_file/random_access_file_test.h b/runtime/base/unix_file/random_access_file_test.h
index 0002433..e7ace4c 100644
--- a/runtime/base/unix_file/random_access_file_test.h
+++ b/runtime/base/unix_file/random_access_file_test.h
@@ -76,6 +76,8 @@
ASSERT_EQ(content.size(), static_cast<uint64_t>(file->Write(content.data(), content.size(), 0)));
TestReadContent(content, file.get());
+
+ CleanUp(file.get());
}
void TestReadContent(const std::string& content, RandomAccessFile* file) {
@@ -131,6 +133,8 @@
ASSERT_EQ(new_length, file->GetLength());
ASSERT_TRUE(ReadString(file.get(), &new_content));
ASSERT_EQ('\0', new_content[new_length - 1]);
+
+ CleanUp(file.get());
}
void TestWrite() {
@@ -163,6 +167,11 @@
ASSERT_EQ(file->GetLength(), new_length);
ASSERT_TRUE(ReadString(file.get(), &new_content));
ASSERT_EQ(std::string("hello\0hello", new_length), new_content);
+
+ CleanUp(file.get());
+ }
+
+ virtual void CleanUp(RandomAccessFile* file ATTRIBUTE_UNUSED) {
}
protected:
diff --git a/runtime/base/unix_file/random_access_file_utils_test.cc b/runtime/base/unix_file/random_access_file_utils_test.cc
index 6317922..9457d22 100644
--- a/runtime/base/unix_file/random_access_file_utils_test.cc
+++ b/runtime/base/unix_file/random_access_file_utils_test.cc
@@ -37,14 +37,14 @@
}
TEST_F(RandomAccessFileUtilsTest, BadSrc) {
- FdFile src(-1);
+ FdFile src(-1, false);
StringFile dst;
ASSERT_FALSE(CopyFile(src, &dst));
}
TEST_F(RandomAccessFileUtilsTest, BadDst) {
StringFile src;
- FdFile dst(-1);
+ FdFile dst(-1, false);
// We need some source content to trigger a write.
// Copying an empty file is a no-op.