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.
Change-Id: I9ab03b1653e69f44cc98946dc89d764c3e045dd4
diff --git a/compiler/image_test.cc b/compiler/image_test.cc
index 0aef512..3c28bc2 100644
--- a/compiler/image_test.cc
+++ b/compiler/image_test.cc
@@ -102,13 +102,16 @@
ASSERT_TRUE(success_image);
bool success_fixup = ElfFixup::Fixup(dup_oat.get(), writer.GetOatDataBegin());
ASSERT_TRUE(success_fixup);
+
+ ASSERT_EQ(dup_oat->FlushCloseOrErase(), 0) << "Could not flush and close oat file "
+ << oat_file.GetFilename();
}
{
std::unique_ptr<File> file(OS::OpenFileForReading(image_file.GetFilename().c_str()));
ASSERT_TRUE(file.get() != NULL);
ImageHeader image_header;
- file->ReadFully(&image_header, sizeof(image_header));
+ ASSERT_EQ(file->ReadFully(&image_header, sizeof(image_header)), true);
ASSERT_TRUE(image_header.IsValid());
ASSERT_GE(image_header.GetImageBitmapOffset(), sizeof(image_header));
ASSERT_NE(0U, image_header.GetImageBitmapSize());
diff --git a/compiler/image_writer.cc b/compiler/image_writer.cc
index 871889f..1d9cb95 100644
--- a/compiler/image_writer.cc
+++ b/compiler/image_writer.cc
@@ -144,8 +144,15 @@
CopyAndFixupObjects();
PatchOatCodeAndMethods(oat_file.get());
+
+ // Before flushing, which might fail, release the mutator lock.
Thread::Current()->TransitionFromRunnableToSuspended(kNative);
+ if (oat_file->FlushCloseOrErase() != 0) {
+ LOG(ERROR) << "Failed to flush and close oat file " << oat_filename << " for " << oat_location;
+ return false;
+ }
+
std::unique_ptr<File> image_file(OS::CreateEmptyFile(image_filename.c_str()));
ImageHeader* image_header = reinterpret_cast<ImageHeader*>(image_->Begin());
if (image_file.get() == NULL) {
@@ -154,6 +161,7 @@
}
if (fchmod(image_file->Fd(), 0644) != 0) {
PLOG(ERROR) << "Failed to make image file world readable: " << image_filename;
+ image_file->Erase();
return EXIT_FAILURE;
}
@@ -161,6 +169,7 @@
CHECK_EQ(image_end_, image_header->GetImageSize());
if (!image_file->WriteFully(image_->Begin(), image_end_)) {
PLOG(ERROR) << "Failed to write image file " << image_filename;
+ image_file->Erase();
return false;
}
@@ -170,9 +179,14 @@
image_header->GetImageBitmapSize(),
image_header->GetImageBitmapOffset())) {
PLOG(ERROR) << "Failed to write image file " << image_filename;
+ image_file->Erase();
return false;
}
+ if (image_file->FlushCloseOrErase() != 0) {
+ PLOG(ERROR) << "Failed to flush and close image file " << image_filename;
+ return false;
+ }
return true;
}
diff --git a/dex2oat/dex2oat.cc b/dex2oat/dex2oat.cc
index 853f103..b0079a2 100644
--- a/dex2oat/dex2oat.cc
+++ b/dex2oat/dex2oat.cc
@@ -434,7 +434,11 @@
// Flush result to disk. Patching code will re-open the file (mmap), so ensure that our view
// of the file already made it there and won't be re-ordered with writes from PatchOat or
// image patching.
- oat_file->Flush();
+ if (oat_file->Flush() != 0) {
+ PLOG(ERROR) << "Failed flushing oat file " << oat_file->GetPath();
+ oat_file->Erase();
+ return nullptr;
+ }
if (!driver->IsImage() && driver->GetCompilerOptions().GetIncludePatchInformation()) {
t2.NewTiming("Patching ELF");
@@ -466,15 +470,23 @@
oat_data_begin = image_writer.GetOatDataBegin();
}
- std::unique_ptr<File> oat_file(OS::OpenFileReadWrite(oat_filename.c_str()));
- if (oat_file.get() == nullptr) {
- PLOG(ERROR) << "Failed to open ELF file: " << oat_filename;
- return false;
- }
+
// Do not fix up the ELF file if we are --compile-pic
if (!compiler_options_->GetCompilePic()) {
+ std::unique_ptr<File> oat_file(OS::OpenFileReadWrite(oat_filename.c_str()));
+ if (oat_file.get() == nullptr) {
+ PLOG(ERROR) << "Failed to open ELF file: " << oat_filename;
+ return false;
+ }
+
if (!ElfFixup::Fixup(oat_file.get(), oat_data_begin)) {
LOG(ERROR) << "Failed to fixup ELF file " << oat_file->GetPath();
+ oat_file->Erase();
+ return false;
+ }
+
+ if (oat_file->FlushCloseOrErase() != 0) {
+ PLOG(ERROR) << "Failed to flush and close patched oat file " << oat_filename;
return false;
}
}
@@ -1258,9 +1270,11 @@
oat_location = oat_filename;
}
} else {
- oat_file.reset(new File(oat_fd, oat_location));
+ oat_file.reset(new File(oat_fd, oat_location, true));
oat_file->DisableAutoClose();
- oat_file->SetLength(0);
+ if (oat_file->SetLength(0)) { // Only warn for truncation error.
+ PLOG(WARNING) << "Truncating oat file " << oat_location << " failed.";
+ }
}
if (oat_file.get() == nullptr) {
PLOG(ERROR) << "Failed to create oat file: " << oat_location;
@@ -1410,7 +1424,10 @@
<< ". Try: adb shell chmod 777 /data/local/tmp";
continue;
}
- tmp_file->WriteFully(dex_file->Begin(), dex_file->Size());
+ // This is just dumping files for debugging. Ignore errors, and leave remnants.
+ UNUSED(tmp_file->WriteFully(dex_file->Begin(), dex_file->Size()));
+ UNUSED(tmp_file->Flush());
+ UNUSED(tmp_file->Close());
LOG(INFO) << "Wrote input to " << tmp_file_name;
}
}
@@ -1481,8 +1498,16 @@
return EXIT_FAILURE;
}
- VLOG(compiler) << "Oat file written successfully (unstripped): " << oat_location;
+ if (!kUsePortableCompiler) {
+ if (oat_file->FlushCloseOrErase() != 0) {
+ PLOG(ERROR) << "Failed to flush and close oat file: " << oat_location;
+ timings.EndTiming();
+ return EXIT_FAILURE;
+ }
+ oat_file.reset();
+ }
+ VLOG(compiler) << "Oat file written successfully (unstripped): " << oat_location;
// Notes on the interleaving of creating the image and oat file to
// ensure the references between the two are correct.
//
@@ -1562,8 +1587,14 @@
// We need to strip after image creation because FixupElf needs to use .strtab.
if (oat_unstripped != oat_stripped) {
TimingLogger::ScopedTiming t("dex2oat OatFile copy", &timings);
- oat_file.reset();
- std::unique_ptr<File> in(OS::OpenFileForReading(oat_unstripped.c_str()));
+ if (kUsePortableCompiler) {
+ if (oat_file->FlushCloseOrErase() != 0) {
+ PLOG(ERROR) << "Failed to flush and close oat file: " << oat_location;
+ return EXIT_FAILURE;
+ }
+ oat_file.reset();
+ }
+ std::unique_ptr<File> in(OS::OpenFileForReading(oat_unstripped.c_str()));
std::unique_ptr<File> out(OS::CreateEmptyFile(oat_stripped.c_str()));
size_t buffer_size = 8192;
std::unique_ptr<uint8_t> buffer(new uint8_t[buffer_size]);
@@ -1579,22 +1610,34 @@
VLOG(compiler) << "Oat file copied successfully (stripped): " << oat_stripped;
}
-#if ART_USE_PORTABLE_COMPILER // We currently only generate symbols on Portable
- if (!compiler_options.GetIncludeDebugSymbols()) {
- timings.NewSplit("dex2oat ElfStripper");
- // Strip unneeded sections for target
- off_t seek_actual = lseek(oat_file->Fd(), 0, SEEK_SET);
- CHECK_EQ(0, seek_actual);
- std::string error_msg;
- CHECK(ElfStripper::Strip(oat_file.get(), &error_msg)) << error_msg;
+ if (kUsePortableCompiler) {
+ if (!compiler_options->GetIncludeDebugSymbols()) {
+ timings.NewTiming("dex2oat ElfStripper");
+ // Strip unneeded sections for target
+ off_t seek_actual = lseek(oat_file->Fd(), 0, SEEK_SET);
+ CHECK_EQ(0, seek_actual);
+ std::string error_msg;
+ CHECK(ElfStripper::Strip(oat_file.get(), &error_msg)) << error_msg;
- // We wrote the oat file successfully, and want to keep it.
- VLOG(compiler) << "Oat file written successfully (stripped): " << oat_location;
- } else {
- VLOG(compiler) << "Oat file written successfully without stripping: " << oat_location;
+ // We wrote the oat file successfully, and want to keep it.
+ VLOG(compiler) << "Oat file written successfully (stripped): " << oat_location;
+ } else {
+ VLOG(compiler) << "Oat file written successfully without stripping: " << oat_location;
+ }
+ if (oat_file->FlushCloseOrErase() != 0) {
+ LOG(ERROR) << "Failed to flush and close oat file: " << oat_location;
+ return EXIT_FAILURE;
+ }
+ oat_file.reset(nullptr);
}
-#endif // ART_USE_PORTABLE_COMPILER
+
+ if (oat_file.get() != nullptr) {
+ if (oat_file->FlushCloseOrErase() != 0) {
+ PLOG(ERROR) << "Failed to flush and close oat file: " << oat_location << "/" << oat_filename;
+ return EXIT_FAILURE;
+ }
+ }
timings.EndTiming();
diff --git a/patchoat/patchoat.cc b/patchoat/patchoat.cc
index b9637d0..0d013d8 100644
--- a/patchoat/patchoat.cc
+++ b/patchoat/patchoat.cc
@@ -895,6 +895,20 @@
}
}
+// Either try to close the file (close=true), or erase it.
+static bool FinishFile(File* file, bool close) {
+ if (close) {
+ if (file->FlushCloseOrErase() != 0) {
+ PLOG(ERROR) << "Failed to flush and close file.";
+ return false;
+ }
+ return true;
+ } else {
+ file->Erase();
+ return false;
+ }
+}
+
static int patchoat(int argc, char **argv) {
InitLogging(argv);
MemMap::Init();
@@ -1166,7 +1180,7 @@
if (output_image_filename.empty()) {
output_image_filename = "output-image-file";
}
- output_image.reset(new File(output_image_fd, output_image_filename));
+ output_image.reset(new File(output_image_fd, output_image_filename, true));
} else {
CHECK(!output_image_filename.empty());
output_image.reset(CreateOrOpen(output_image_filename.c_str(), &new_image_out));
@@ -1180,7 +1194,7 @@
if (input_oat_filename.empty()) {
input_oat_filename = "input-oat-file";
}
- input_oat.reset(new File(input_oat_fd, input_oat_filename));
+ input_oat.reset(new File(input_oat_fd, input_oat_filename, false));
if (input_oat == nullptr) {
// Unlikely, but ensure exhaustive logging in non-0 exit code case
LOG(ERROR) << "Failed to open input oat file by its FD" << input_oat_fd;
@@ -1199,7 +1213,7 @@
if (output_oat_filename.empty()) {
output_oat_filename = "output-oat-file";
}
- output_oat.reset(new File(output_oat_fd, output_oat_filename));
+ output_oat.reset(new File(output_oat_fd, output_oat_filename, true));
if (output_oat == nullptr) {
// Unlikely, but ensure exhaustive logging in non-0 exit code case
LOG(ERROR) << "Failed to open output oat file by its FD" << output_oat_fd;
@@ -1272,14 +1286,20 @@
output_oat.get(), output_image.get(), isa, &timings,
output_oat_fd >= 0, // was it opened from FD?
new_oat_out);
+ // The order here doesn't matter. If the first one is successfully saved and the second one
+ // erased, ImageSpace will still detect a problem and not use the files.
+ ret = ret && FinishFile(output_image.get(), ret);
+ ret = ret && FinishFile(output_oat.get(), ret);
} else if (have_oat_files) {
TimingLogger::ScopedTiming pt("patch oat", &timings);
ret = PatchOat::Patch(input_oat.get(), base_delta, output_oat.get(), &timings,
output_oat_fd >= 0, // was it opened from FD?
new_oat_out);
+ ret = ret && FinishFile(output_oat.get(), ret);
} else if (have_image_files) {
TimingLogger::ScopedTiming pt("patch image", &timings);
ret = PatchOat::Patch(input_image_location, base_delta, output_image.get(), isa, &timings);
+ ret = ret && FinishFile(output_image.get(), ret);
} else {
CHECK(false);
ret = true;
diff --git a/runtime/Android.mk b/runtime/Android.mk
index 17ee8ab..0f5050b 100644
--- a/runtime/Android.mk
+++ b/runtime/Android.mk
@@ -295,6 +295,7 @@
arch/x86_64/registers_x86_64.h \
base/allocator.h \
base/mutex.h \
+ base/unix_file/fd_file.h \
dex_file.h \
dex_instruction.h \
gc/collector/gc_type.h \
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..3d38e67 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(ERROR) << "File " << file_path_ << " wasn't explicitly flushed before destruction.";
+ }
+ if (guard_state_ < GuardState::kClosed) {
+ LOG(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(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(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(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(ERROR) << "CloseOrErase failed while flushing a file.";
+ Erase();
+ return flush_result;
+ }
+ int close_result = TEMP_FAILURE_RETRY(Close());
+ if (close_result != 0) {
+ LOG(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(ERROR) << "FlushClose failed while flushing a file.";
+ }
+ int close_result = TEMP_FAILURE_RETRY(Close());
+ if (close_result != 0) {
+ LOG(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/mapped_file.cc b/runtime/base/unix_file/mapped_file.cc
index 63927b1..ad43382 100644
--- a/runtime/base/unix_file/mapped_file.cc
+++ b/runtime/base/unix_file/mapped_file.cc
@@ -14,8 +14,8 @@
* limitations under the License.
*/
-#include "base/logging.h"
#include "base/unix_file/mapped_file.h"
+
#include <fcntl.h>
#include <sys/mman.h>
#include <sys/stat.h>
@@ -24,6 +24,8 @@
#include <algorithm>
#include <string>
+#include "base/logging.h"
+
namespace unix_file {
MappedFile::~MappedFile() {
@@ -39,6 +41,10 @@
bool MappedFile::MapReadOnly() {
CHECK(IsOpened());
CHECK(!IsMapped());
+
+ // Mapping readonly means we don't need to enforce Flush and Close.
+ resetGuard(GuardState::kNoCheck);
+
struct stat st;
int result = TEMP_FAILURE_RETRY(fstat(Fd(), &st));
if (result == -1) {
@@ -71,6 +77,10 @@
<< "' to size " << file_size;
return false;
}
+
+ // Need to track this now.
+ resetGuard(GuardState::kBase);
+
file_size_ = file_size;
do {
mapped_file_ =
@@ -130,6 +140,7 @@
}
int MappedFile::Flush() {
+ moveUp(GuardState::kFlushed, "Flushing closed file.");
int rc = IsMapped() ? TEMP_FAILURE_RETRY(msync(mapped_file_, file_size_, 0)) : FdFile::Flush();
return rc == -1 ? -errno : 0;
}
@@ -145,6 +156,7 @@
std::min(byte_count, file_size_ - offset));
if (write_size > 0) {
memcpy(data() + offset, buf, write_size);
+ moveTo(GuardState::kBase, GuardState::kClosed, "Writing into a closed file.");
}
return write_size;
} else {
diff --git a/runtime/base/unix_file/mapped_file.h b/runtime/base/unix_file/mapped_file.h
index 73056e9..4ff259b 100644
--- a/runtime/base/unix_file/mapped_file.h
+++ b/runtime/base/unix_file/mapped_file.h
@@ -41,11 +41,12 @@
#endif
};
- MappedFile() : FdFile(), file_size_(-1), mapped_file_(NULL) {
+ MappedFile() : FdFile(), file_size_(-1), mapped_file_(NULL), map_mode_(kMapReadOnly) {
}
// Creates a MappedFile using the given file descriptor. Takes ownership of
// the file descriptor.
- explicit MappedFile(int fd) : FdFile(fd), file_size_(-1), mapped_file_(NULL) {
+ explicit MappedFile(int fd, bool check_usage) : FdFile(fd, check_usage), file_size_(-1),
+ mapped_file_(NULL), map_mode_(kMapReadOnly) {
}
// Unmaps and closes the file if needed.
diff --git a/runtime/base/unix_file/mapped_file_test.cc b/runtime/base/unix_file/mapped_file_test.cc
index 59334d4..0455df6 100644
--- a/runtime/base/unix_file/mapped_file_test.cc
+++ b/runtime/base/unix_file/mapped_file_test.cc
@@ -15,6 +15,7 @@
*/
#include "base/unix_file/mapped_file.h"
+#include "base/casts.h"
#include "base/logging.h"
#include "base/unix_file/fd_file.h"
#include "base/unix_file/random_access_file_test.h"
@@ -34,12 +35,13 @@
good_path_ = GetTmpPath("some-file.txt");
int fd = TEMP_FAILURE_RETRY(open(good_path_.c_str(), O_CREAT|O_RDWR, 0666));
- FdFile dst(fd);
+ FdFile dst(fd, false);
StringFile src;
src.Assign(kContent);
ASSERT_TRUE(CopyFile(src, &dst));
+ ASSERT_EQ(dst.FlushClose(), 0);
}
void TearDown() {
@@ -55,6 +57,15 @@
return f;
}
+ void CleanUp(RandomAccessFile* file) OVERRIDE {
+ if (file == nullptr) {
+ return;
+ }
+ MappedFile* f = ::art::down_cast<MappedFile*>(file);
+ UNUSED(f->Flush());
+ UNUSED(f->Close());
+ }
+
const std::string kContent;
std::string good_path_;
};
@@ -80,7 +91,7 @@
TEST_F(MappedFileTest, OpenFdClose) {
FILE* f = tmpfile();
ASSERT_TRUE(f != NULL);
- MappedFile file(fileno(f));
+ MappedFile file(fileno(f), false);
EXPECT_GE(file.Fd(), 0);
EXPECT_TRUE(file.IsOpened());
EXPECT_EQ(0, file.Close());
@@ -108,6 +119,7 @@
ASSERT_TRUE(file.data());
EXPECT_EQ(kContent[0], *file.data());
EXPECT_EQ(0, file.Flush());
+ file.Close();
}
TEST_F(MappedFileTest, CanWriteNewData) {
@@ -122,10 +134,11 @@
EXPECT_EQ(kContent.size(), static_cast<uint64_t>(file.size()));
ASSERT_TRUE(file.data());
memcpy(file.data(), kContent.c_str(), kContent.size());
+ EXPECT_EQ(0, file.Flush());
EXPECT_EQ(0, file.Close());
EXPECT_FALSE(file.IsMapped());
- FdFile new_file(TEMP_FAILURE_RETRY(open(new_path.c_str(), O_RDONLY)));
+ FdFile new_file(TEMP_FAILURE_RETRY(open(new_path.c_str(), O_RDONLY)), false);
StringFile buffer;
ASSERT_TRUE(CopyFile(new_file, &buffer));
EXPECT_EQ(kContent, buffer.ToStringPiece());
@@ -192,6 +205,7 @@
ASSERT_TRUE(file.Open(good_path_, MappedFile::kReadWriteMode));
ASSERT_TRUE(file.MapReadWrite(kContent.size()));
TestReadContent(kContent, &file);
+ UNUSED(file.FlushClose());
}
TEST_F(MappedFileTest, WriteMappedReadWrite) {
@@ -217,6 +231,7 @@
EXPECT_EQ(kContent.size(),
static_cast<uint64_t>(file.Write(kContent.c_str(), kContent.size(), 0)));
EXPECT_EQ(0, memcmp(kContent.c_str(), file.data(), kContent.size()));
+ UNUSED(file.FlushClose());
}
#if 0 // death tests don't work on android yet
diff --git a/runtime/base/unix_file/random_access_file_test.h b/runtime/base/unix_file/random_access_file_test.h
index 0002433..7734dc4 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) {
}
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.
diff --git a/runtime/common_runtime_test.cc b/runtime/common_runtime_test.cc
index e9e11f6..877d327 100644
--- a/runtime/common_runtime_test.cc
+++ b/runtime/common_runtime_test.cc
@@ -59,7 +59,7 @@
filename_ += "/TmpFile-XXXXXX";
int fd = mkstemp(&filename_[0]);
CHECK_NE(-1, fd);
- file_.reset(new File(fd, GetFilename()));
+ file_.reset(new File(fd, GetFilename(), true));
}
ScratchFile::ScratchFile(const ScratchFile& other, const char* suffix) {
@@ -67,7 +67,7 @@
filename_ += suffix;
int fd = open(filename_.c_str(), O_RDWR | O_CREAT, 0666);
CHECK_NE(-1, fd);
- file_.reset(new File(fd, GetFilename()));
+ file_.reset(new File(fd, GetFilename(), true));
}
ScratchFile::ScratchFile(File* file) {
@@ -88,6 +88,11 @@
if (!OS::FileExists(filename_.c_str())) {
return;
}
+ if (file_.get() != nullptr) {
+ if (file_->FlushCloseOrErase() != 0) {
+ PLOG(WARNING) << "Error closing scratch file.";
+ }
+ }
int unlink_result = unlink(filename_.c_str());
CHECK_EQ(0, unlink_result);
}
diff --git a/runtime/dex_file_test.cc b/runtime/dex_file_test.cc
index d0c5603..d5304e7 100644
--- a/runtime/dex_file_test.cc
+++ b/runtime/dex_file_test.cc
@@ -146,6 +146,9 @@
if (!file->WriteFully(dex_bytes.get(), length)) {
PLOG(FATAL) << "Failed to write base64 as dex file";
}
+ if (file->FlushCloseOrErase() != 0) {
+ PLOG(FATAL) << "Could not flush and close test file.";
+ }
file.reset();
// read dex file
diff --git a/runtime/dex_file_verifier_test.cc b/runtime/dex_file_verifier_test.cc
index d475d42..7aff7dc 100644
--- a/runtime/dex_file_verifier_test.cc
+++ b/runtime/dex_file_verifier_test.cc
@@ -115,6 +115,9 @@
if (!file->WriteFully(dex_bytes.get(), length)) {
PLOG(FATAL) << "Failed to write base64 as dex file";
}
+ if (file->FlushCloseOrErase() != 0) {
+ PLOG(FATAL) << "Could not flush and close test file.";
+ }
file.reset();
// read dex file
@@ -177,6 +180,9 @@
if (!file->WriteFully(bytes, length)) {
PLOG(FATAL) << "Failed to write base64 as dex file";
}
+ if (file->FlushCloseOrErase() != 0) {
+ PLOG(FATAL) << "Could not flush and close test file.";
+ }
file.reset();
// read dex file
diff --git a/runtime/gc/space/image_space.cc b/runtime/gc/space/image_space.cc
index 5f8fb0d..f765f0e 100644
--- a/runtime/gc/space/image_space.cc
+++ b/runtime/gc/space/image_space.cc
@@ -132,6 +132,11 @@
VLOG(startup) << "Creating boot start marker: " << boot_marker;
std::unique_ptr<File> f(OS::CreateEmptyFile(boot_marker.c_str()));
+ if (f.get() != nullptr) {
+ if (f->FlushCloseOrErase() != 0) {
+ PLOG(WARNING) << "Failed to write boot marker.";
+ }
+ }
}
static bool GenerateImage(const std::string& image_filename, InstructionSet image_isa,
diff --git a/runtime/hprof/hprof.cc b/runtime/hprof/hprof.cc
index fd67197..a2cccc5 100644
--- a/runtime/hprof/hprof.cc
+++ b/runtime/hprof/hprof.cc
@@ -475,9 +475,14 @@
}
}
- std::unique_ptr<File> file(new File(out_fd, filename_));
+ std::unique_ptr<File> file(new File(out_fd, filename_, true));
okay = file->WriteFully(header_data_ptr_, header_data_size_) &&
- file->WriteFully(body_data_ptr_, body_data_size_);
+ file->WriteFully(body_data_ptr_, body_data_size_);
+ if (okay) {
+ okay = file->FlushCloseOrErase() == 0;
+ } else {
+ file->Erase();
+ }
if (!okay) {
std::string msg(StringPrintf("Couldn't dump heap; writing \"%s\" failed: %s",
filename_.c_str(), strerror(errno)));
diff --git a/runtime/signal_catcher.cc b/runtime/signal_catcher.cc
index 11e06fe..916e1e2 100644
--- a/runtime/signal_catcher.cc
+++ b/runtime/signal_catcher.cc
@@ -108,11 +108,17 @@
PLOG(ERROR) << "Unable to open stack trace file '" << stack_trace_file_ << "'";
return;
}
- std::unique_ptr<File> file(new File(fd, stack_trace_file_));
- if (!file->WriteFully(s.data(), s.size())) {
- PLOG(ERROR) << "Failed to write stack traces to '" << stack_trace_file_ << "'";
+ std::unique_ptr<File> file(new File(fd, stack_trace_file_, true));
+ bool success = file->WriteFully(s.data(), s.size());
+ if (success) {
+ success = file->FlushCloseOrErase() == 0;
} else {
+ file->Erase();
+ }
+ if (success) {
LOG(INFO) << "Wrote stack traces to '" << stack_trace_file_ << "'";
+ } else {
+ PLOG(ERROR) << "Failed to write stack traces to '" << stack_trace_file_ << "'";
}
}
diff --git a/runtime/trace.cc b/runtime/trace.cc
index 4c5e909..17ede83 100644
--- a/runtime/trace.cc
+++ b/runtime/trace.cc
@@ -427,6 +427,15 @@
instrumentation::Instrumentation::kMethodExited |
instrumentation::Instrumentation::kMethodUnwind);
}
+ if (the_trace->trace_file_.get() != nullptr) {
+ // Do not try to erase, so flush and close explicitly.
+ if (the_trace->trace_file_->Flush() != 0) {
+ PLOG(ERROR) << "Could not flush trace file.";
+ }
+ if (the_trace->trace_file_->Close() != 0) {
+ PLOG(ERROR) << "Could not close trace file.";
+ }
+ }
delete the_trace;
}
runtime->GetThreadList()->ResumeAll();
diff --git a/runtime/zip_archive_test.cc b/runtime/zip_archive_test.cc
index 96abee2..70a4dda 100644
--- a/runtime/zip_archive_test.cc
+++ b/runtime/zip_archive_test.cc
@@ -41,7 +41,7 @@
ScratchFile tmp;
ASSERT_NE(-1, tmp.GetFd());
- std::unique_ptr<File> file(new File(tmp.GetFd(), tmp.GetFilename()));
+ std::unique_ptr<File> file(new File(tmp.GetFd(), tmp.GetFilename(), false));
ASSERT_TRUE(file.get() != NULL);
bool success = zip_entry->ExtractToFile(*file, &error_msg);
ASSERT_TRUE(success) << error_msg;