Skip to content

Commit

Permalink
Delete the temporary install base if rename fails
Browse files Browse the repository at this point in the history
The extraction process creates a directory $install_base.tmp.XXXX, extracts
the necessary install data into it, then moves it to $install_base. Typically
the rename can fail if multiple instances race to create $install_base, but it
can also fail for less predictable reasons. In either case, clean up the temp
directory to avoid leaving unused data behind.

Note that we can still leave a partial directory behind if something goes wrong
with extracting data into the temp dir. This'll be a little harder to clean up,
or alternatively it might also be something we want to leave in place for
inspection after the fact.

Windows implementation will come separately. I should be able to throw something
together from the existing windows UnlinkPath and DeleteAllUnder
(windows_test_util), but I lack a windows machine to able to move quickly, so
I want to make sure this first pass sticks.

RELNOTES: None
PiperOrigin-RevId: 292599669
  • Loading branch information
michajlo authored and copybara-github committed Jan 31, 2020
1 parent 2d40d5c commit 869bd70
Show file tree
Hide file tree
Showing 6 changed files with 128 additions and 0 deletions.
2 changes: 2 additions & 0 deletions src/main/cpp/blaze.cc
Original file line number Diff line number Diff line change
Expand Up @@ -998,6 +998,7 @@ static DurationMillis ExtractData(const string &self_path,
result == blaze_util::kRenameDirectoryFailureNotEmpty) {
// If renaming fails because the directory already exists and is not
// empty, then we assume another good installation snuck in before us.
blaze_util::RemoveRecursively(tmp_install);
break;
} else {
// Otherwise the install directory may still be scanned by the antivirus
Expand All @@ -1013,6 +1014,7 @@ static DurationMillis ExtractData(const string &self_path,

// Give up renaming after 120 failed attempts / 2 minutes.
if (attempts == 120) {
blaze_util::RemoveRecursively(tmp_install);
BAZEL_DIE(blaze_exit_code::LOCAL_ENVIRONMENTAL_ERROR)
<< "install base directory '" << tmp_install
<< "' could not be renamed into place: " << GetLastErrorString();
Expand Down
5 changes: 5 additions & 0 deletions src/main/cpp/util/file_platform.h
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,11 @@ void SyncFile(const Path &path);
bool MakeDirectories(const std::string &path, unsigned int mode);
bool MakeDirectories(const Path &path, unsigned int mode);

// Removes the specified path or directory, and in the latter case, all of its
// contents. Returns true iff the path doesn't exists when the method completes
// (including if the path didn't exist to begin with). Does not follow symlinks.
bool RemoveRecursively(const std::string &path);

// Returns the current working directory.
// The path is platform-specific (e.g. Windows path of Windows) and absolute.
std::string GetCwd();
Expand Down
39 changes: 39 additions & 0 deletions src/main/cpp/util/file_posix.cc
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,45 @@ static bool MakeDirectories(const string &path, mode_t mode, bool childmost) {
return stat_succeeded;
}

static bool RemoveDirRecursively(const std::string &path) {
DIR *dir;
if ((dir = opendir(path.c_str())) == NULL) {
return false;
}

struct dirent *ent;
while ((ent = readdir(dir)) != NULL) {
if (!strcmp(ent->d_name, ".") || !strcmp(ent->d_name, "..")) {
continue;
}

if (!RemoveRecursively(blaze_util::JoinPath(path, ent->d_name))) {
closedir(dir);
return false;
}
}

if (closedir(dir) != 0) {
return false;
}

return rmdir(path.c_str()) == 0;
}

bool RemoveRecursively(const std::string &path) {
struct stat stat_buf;
if (lstat(path.c_str(), &stat_buf) == -1) {
// Non-existent is good enough.
return errno == ENOENT;
}

if (S_ISDIR(stat_buf.st_mode) && !S_ISLNK(stat_buf.st_mode)) {
return RemoveDirRecursively(path);
} else {
return UnlinkPath(path);
}
}

class PosixPipe : public IPipe {
public:
PosixPipe(int recv_socket, int send_socket)
Expand Down
5 changes: 5 additions & 0 deletions src/main/cpp/util/file_windows.cc
Original file line number Diff line number Diff line change
Expand Up @@ -646,6 +646,11 @@ bool MakeDirectories(const Path& path, unsigned int mode) {
return MakeDirectoriesW(path.AsNativePath(), mode);
}

bool RemoveRecursively(const string &path) {
// TODO(bazel-team): Implement this.
return false;
}

static inline void ToLowerW(WCHAR* p) {
while (*p) {
*p++ = towlower(*p);
Expand Down
63 changes: 63 additions & 0 deletions src/test/cpp/util/file_posix_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -322,4 +322,67 @@ TEST(FilePosixTest, ForEachDirectoryEntry) {
rmdir(root.c_str());
}

TEST(FileTest, TestRemoveRecursively) {
const char* tempdir_cstr = getenv("TEST_TMPDIR");
ASSERT_NE(tempdir_cstr, nullptr);
string tempdir(tempdir_cstr);
ASSERT_TRUE(PathExists(tempdir));

string non_existent_dir(JoinPath(tempdir, "test_rmr_non_existent"));
EXPECT_TRUE(RemoveRecursively(non_existent_dir));
EXPECT_FALSE(PathExists(non_existent_dir));

string empty_dir(JoinPath(tempdir, "test_rmr_empty_dir"));
EXPECT_TRUE(MakeDirectories(empty_dir, 0700));
EXPECT_TRUE(RemoveRecursively(empty_dir));
EXPECT_FALSE(PathExists(empty_dir));

string dir_with_content(JoinPath(tempdir, "test_rmr_dir_w_content"));
EXPECT_TRUE(MakeDirectories(dir_with_content, 0700));
EXPECT_TRUE(WriteFile("junkdata", 8, JoinPath(dir_with_content, "file")));
EXPECT_TRUE(MakeDirectories(JoinPath(dir_with_content, "dir"), 0700));
EXPECT_TRUE(RemoveRecursively(dir_with_content));
EXPECT_FALSE(PathExists(dir_with_content));

string regular_file(JoinPath(tempdir, "test_rmr_regular_file"));
EXPECT_TRUE(WriteFile("junkdata", 8, regular_file));
EXPECT_TRUE(RemoveRecursively(regular_file));
EXPECT_FALSE(PathExists(regular_file));

string unwritable_dir(JoinPath(tempdir, "test_rmr_unwritable"));
EXPECT_TRUE(MakeDirectories(unwritable_dir, 0700));
EXPECT_TRUE(WriteFile("junkdata", 8, JoinPath(unwritable_dir, "file")));
ASSERT_EQ(0, chmod(unwritable_dir.c_str(), 0500));
EXPECT_FALSE(RemoveRecursively(unwritable_dir));

string symlink_target_dir(JoinPath(tempdir, "test_rmr_symlink_target_dir"));
EXPECT_TRUE(MakeDirectories(symlink_target_dir, 0700));
string symlink_target_dir_file(JoinPath(symlink_target_dir, "file"));
EXPECT_TRUE(WriteFile("junkdata", 8, symlink_target_dir_file));
string symlink_dir(JoinPath(tempdir, "test_rmr_symlink_dir"));
EXPECT_EQ(0, symlink(symlink_target_dir.c_str(), symlink_dir.c_str()));
EXPECT_TRUE(RemoveRecursively(symlink_dir));
EXPECT_FALSE(PathExists(symlink_dir));
EXPECT_TRUE(PathExists(symlink_target_dir));
EXPECT_TRUE(PathExists(symlink_target_dir_file));

string dir_with_symlinks(JoinPath(tempdir, "test_rmr_dir_w_symlinks"));
EXPECT_TRUE(MakeDirectories(dir_with_symlinks, 0700));
string file_symlink_target(JoinPath(tempdir, "test_rmr_dir_w_symlinks_file"));
EXPECT_TRUE(WriteFile("junkdata", 8, file_symlink_target));
EXPECT_EQ(0, symlink(file_symlink_target.c_str(),
JoinPath(dir_with_symlinks, "file").c_str()));
string dir_symlink_target(JoinPath(tempdir, "test_rmr_dir_w_symlinks_dir"));
EXPECT_TRUE(MakeDirectories(dir_symlink_target, 0700));
string dir_symlink_target_file(JoinPath(dir_symlink_target, "file"));
EXPECT_TRUE(WriteFile("junkdata", 8, dir_symlink_target_file));
EXPECT_EQ(0, symlink(dir_symlink_target.c_str(),
JoinPath(dir_with_symlinks, "dir").c_str()));
EXPECT_TRUE(RemoveRecursively(dir_with_symlinks));
EXPECT_FALSE(PathExists(dir_with_symlinks));
EXPECT_TRUE(PathExists(dir_symlink_target));
EXPECT_TRUE(PathExists(dir_symlink_target_file));
EXPECT_TRUE(PathExists(file_symlink_target));
}

} // namespace blaze_util
14 changes: 14 additions & 0 deletions src/test/shell/integration/client_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,20 @@ function test_nonwritable_output_base() {
expect_log "FATAL.* Output base directory '/' must be readable and writable."
}

function test_install_base_races_dont_leave_temp_files() {
declare -a client_pids
for i in {1..3}; do
bazel --install_base="$TEST_TMPDIR/race/install" \
--output_base="$TEST_TMPDIR/out$i" info install_base &
client_pids+=($!)
done
for pid in "${client_pids[@]}"; do
wait $pid
done
# Expect "install" to be the only thing in the "race" directory.
assert_equals "install" "$(ls "$TEST_TMPDIR/race/")"
}

function test_no_arguments() {
bazel >&$TEST_log || fail "Expected zero exit"
expect_log "Usage: b\\(laze\\|azel\\)"
Expand Down

0 comments on commit 869bd70

Please sign in to comment.