Skip to content

Commit d64703f

Browse files
committed
Fix delete_directory_recursive not deleting directories sometimes
If a test creates a non-empty untraversable directory (test_configuration_loader.making_parent_dir_unreadable_is_detected_as_change is an example), delete_directory_recursive fails to delete the directory. This leaves /tmp cluttered with quick-lint-js test junk. Make delete_directory_recursive delete untraversable directories by making them traversable with chmod().
1 parent 6e102d0 commit d64703f

File tree

4 files changed

+124
-2
lines changed

4 files changed

+124
-2
lines changed

src/temporary-directory.cpp

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -95,9 +95,16 @@ void delete_directory_recursive(const std::string &path) {
9595
}
9696
while (::FTSENT *entry = ::fts_read(fts)) {
9797
switch (entry->fts_info) {
98-
case FTS_D:
99-
// Do nothing. We handle FTS_DP (post-order) instead.
98+
case FTS_D: {
99+
// Make sure the directory is traversable before traversing.
100+
int rc = ::chmod(entry->fts_accpath, 0700);
101+
if (rc != 0) {
102+
std::fprintf(stderr,
103+
"warning: failed to change permissions for %s: %s\n",
104+
entry->fts_accpath, std::strerror(errno));
105+
}
100106
break;
107+
}
101108

102109
case FTS_DP: {
103110
int rc = ::rmdir(entry->fts_accpath);

test/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,7 @@ quick_lint_js_add_executable(
7777
test-pipe-writer.cpp
7878
test-result.cpp
7979
test-string-view.cpp
80+
test-temporary-directory.cpp
8081
test-text-error-reporter.cpp
8182
test-translation.cpp
8283
test-uri.cpp

test/quick-lint-js/file-matcher.h

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,9 @@
2323
#define EXPECT_SAME_FILE(path_a, path_b) \
2424
EXPECT_PRED_FORMAT2(::quick_lint_js::assert_same_file, path_a, path_b)
2525

26+
#define EXPECT_FILE_DOES_NOT_EXIST(path) \
27+
EXPECT_PRED_FORMAT1(::quick_lint_js::assert_file_does_not_exist, path)
28+
2629
namespace quick_lint_js {
2730
inline ::testing::AssertionResult assert_same_file(const char* lhs_expr,
2831
const char* rhs_expr,
@@ -88,6 +91,43 @@ inline ::testing::AssertionResult assert_same_file(
8891
const std::string& rhs_path) {
8992
return assert_same_file(lhs_expr, rhs_expr, lhs_path.c_str(), rhs_path);
9093
}
94+
95+
inline ::testing::AssertionResult assert_file_does_not_exist(const char* expr,
96+
const char* path) {
97+
bool exists;
98+
#if QLJS_HAVE_STD_FILESYSTEM
99+
exists = std::filesystem::exists(std::filesystem::path(path));
100+
#elif QLJS_HAVE_SYS_STAT_H
101+
struct ::stat s;
102+
if (::stat(path, &s) == 0) {
103+
exists = true;
104+
} else {
105+
switch (errno) {
106+
case ENOENT:
107+
exists = false;
108+
break;
109+
default:
110+
return ::testing::AssertionFailure()
111+
<< "checking for existance of " << expr << " (" << path
112+
<< ") failed: " << std::strerror(errno);
113+
}
114+
}
115+
#else
116+
#error "Unsupported platform"
117+
#endif
118+
if (exists) {
119+
return ::testing::AssertionFailure()
120+
<< expr << " (" << path << ") should not exist but it does";
121+
} else {
122+
return ::testing::AssertionSuccess();
123+
}
124+
}
125+
126+
inline ::testing::AssertionResult assert_file_does_not_exist(
127+
const char* expr, const std::string& path) {
128+
return assert_file_does_not_exist(expr, path.c_str());
129+
}
130+
91131
}
92132

93133
#endif

test/test-temporary-directory.cpp

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
// Copyright (C) 2020 Matthew "strager" Glazar
2+
// See end of file for extended copyright information.
3+
4+
#include <cerrno>
5+
#include <cstring>
6+
#include <gtest/gtest.h>
7+
#include <quick-lint-js/file-matcher.h>
8+
#include <quick-lint-js/file.h>
9+
#include <quick-lint-js/temporary-directory.h>
10+
#include <string>
11+
12+
#if QLJS_HAVE_UNISTD_H
13+
#include <unistd.h>
14+
#endif
15+
16+
namespace quick_lint_js {
17+
namespace {
18+
#if QLJS_HAVE_UNISTD_H
19+
TEST(test_temporary_directory, delete_directory_containing_unwritable_file) {
20+
std::string temp_dir = make_temporary_directory();
21+
std::string sub_dir = temp_dir + "/subdir";
22+
create_directory(sub_dir);
23+
std::string unwritable_file = sub_dir + "/unwritable";
24+
write_file(unwritable_file, u8"unwritable file");
25+
EXPECT_EQ(::chmod(unwritable_file.c_str(), 0000), 0)
26+
<< "failed to make " << unwritable_file
27+
<< " inaccessible: " << std::strerror(errno);
28+
29+
delete_directory_recursive(sub_dir);
30+
31+
EXPECT_FILE_DOES_NOT_EXIST(unwritable_file);
32+
EXPECT_FILE_DOES_NOT_EXIST(sub_dir);
33+
}
34+
35+
TEST(test_temporary_directory,
36+
delete_directory_containing_non_empty_untraversable_directory) {
37+
std::string temp_dir = make_temporary_directory();
38+
std::string sub_dir = temp_dir + "/sub_dir";
39+
create_directory(sub_dir);
40+
std::string untraversable_dir = sub_dir + "/untraversable_dir";
41+
create_directory(untraversable_dir);
42+
std::string unfindable_file = untraversable_dir + "/unfindable_file";
43+
write_file(unfindable_file, u8"can't see me!");
44+
EXPECT_EQ(::chmod(untraversable_dir.c_str(), 0600), 0)
45+
<< "failed to make " << untraversable_dir
46+
<< " untraversable: " << std::strerror(errno);
47+
48+
delete_directory_recursive(sub_dir);
49+
50+
EXPECT_FILE_DOES_NOT_EXIST(unfindable_file);
51+
EXPECT_FILE_DOES_NOT_EXIST(untraversable_dir);
52+
EXPECT_FILE_DOES_NOT_EXIST(sub_dir);
53+
}
54+
#endif
55+
}
56+
}
57+
58+
// quick-lint-js finds bugs in JavaScript programs.
59+
// Copyright (C) 2020 Matthew "strager" Glazar
60+
//
61+
// This file is part of quick-lint-js.
62+
//
63+
// quick-lint-js is free software: you can redistribute it and/or modify
64+
// it under the terms of the GNU General Public License as published by
65+
// the Free Software Foundation, either version 3 of the License, or
66+
// (at your option) any later version.
67+
//
68+
// quick-lint-js is distributed in the hope that it will be useful,
69+
// but WITHOUT ANY WARRANTY; without even the implied warranty of
70+
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
71+
// GNU General Public License for more details.
72+
//
73+
// You should have received a copy of the GNU General Public License
74+
// along with quick-lint-js. If not, see <https://www.gnu.org/licenses/>.

0 commit comments

Comments
 (0)