Skip to content

Commit 16558d2

Browse files
author
kirslayk
committed
feat userver core: fix fs::TempFile destructor crash when fs::TempFile object was moved already
commit_hash:6e92edd5bb95a09f9747b3dbb33053c404c9d1a7
1 parent 69f95f2 commit 16558d2

File tree

6 files changed

+144
-3
lines changed

6 files changed

+144
-3
lines changed

‎.mapping.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1572,6 +1572,7 @@
15721572
"core/src/fs/read.cpp":"taxi/uservices/userver/core/src/fs/read.cpp",
15731573
"core/src/fs/read_test.cpp":"taxi/uservices/userver/core/src/fs/read_test.cpp",
15741574
"core/src/fs/temp_file.cpp":"taxi/uservices/userver/core/src/fs/temp_file.cpp",
1575+
"core/src/fs/temp_file_test.cpp":"taxi/uservices/userver/core/src/fs/temp_file_test.cpp",
15751576
"core/src/fs/write.cpp":"taxi/uservices/userver/core/src/fs/write.cpp",
15761577
"core/src/fs/write_test.cpp":"taxi/uservices/userver/core/src/fs/write_test.cpp",
15771578
"core/src/logging/component.cpp":"taxi/uservices/userver/core/src/logging/component.cpp",

‎core/include/userver/fs/temp_file.hpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ class TempFile final {
4040
TempFile() = delete;
4141
TempFile(TempFile&& other) noexcept = default;
4242
TempFile& operator=(TempFile&& other) noexcept = default;
43-
~TempFile();
43+
~TempFile() noexcept;
4444

4545
/// Take ownership of an existing file
4646
static TempFile Adopt(std::string path, engine::TaskProcessor& fs_task_processor);

‎core/src/fs/temp_file.cpp‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
#include <userver/fs/temp_file.hpp>
22

33
#include <userver/engine/async.hpp>
4+
#include <userver/logging/log.hpp>
45

56
USERVER_NAMESPACE_BEGIN
67

@@ -27,7 +28,13 @@ TempFile::Create(std::string_view parent_path, std::string_view name_prefix, eng
2728
};
2829
}
2930

30-
TempFile::~TempFile() { std::move(*this).Remove(); }
31+
TempFile::~TempFile() noexcept {
32+
try {
33+
std::move(*this).Remove();
34+
} catch (const std::exception& ex) {
35+
LOG_ERROR() << "fs::~TempFile failed with exception:" << ex;
36+
}
37+
}
3138

3239
TempFile TempFile::Adopt(std::string path, engine::TaskProcessor& fs_task_processor) {
3340
return {fs_task_processor, blocking::TempFile::Adopt(std::move(path))};

‎core/src/fs/temp_file_test.cpp‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
#include <gmock/gmock.h>
2+
#include <userver/utest/utest.hpp>
3+
4+
#include <boost/filesystem/operations.hpp>
5+
6+
#include <userver/engine/task/current_task.hpp>
7+
#include <userver/fs/blocking/temp_directory.hpp>
8+
#include <userver/fs/temp_file.hpp>
9+
#include <userver/logging/log.hpp>
10+
#include <userver/utest/log_capture_fixture.hpp>
11+
12+
USERVER_NAMESPACE_BEGIN
13+
14+
namespace {
15+
using FsTempFileWithLog = utest::LogCaptureFixture<>;
16+
}
17+
18+
UTEST_F(FsTempFileWithLog, TempFileDestructorWithException) {
19+
std::string parent_dir;
20+
{
21+
const auto parent = fs::blocking::TempDirectory::Create();
22+
parent_dir = parent.GetPath();
23+
24+
{
25+
// Create temp file in temporaty directory
26+
const auto file = fs::TempFile::Create(
27+
parent_dir, "TempFileDestructorWithException", engine::current_task::GetTaskProcessor()
28+
);
29+
30+
const auto dir_status = boost::filesystem::status(parent_dir);
31+
const auto original_perms = dir_status.permissions();
32+
33+
LOG_DEBUG() << "Original directory permissions: " << static_cast<int>(original_perms);
34+
35+
// Check that the directory has write permissions
36+
ASSERT_TRUE((original_perms & boost::filesystem::perms::owner_write) != boost::filesystem::perms::no_perms);
37+
38+
// Change permissions for the directory
39+
boost::filesystem::permissions(
40+
parent_dir,
41+
boost::filesystem::perms::owner_read | boost::filesystem::perms::owner_exe |
42+
boost::filesystem::perms::group_read | boost::filesystem::perms::group_exe |
43+
boost::filesystem::perms::others_read | boost::filesystem::perms::others_exe
44+
);
45+
LOG_DEBUG() << "Changed directory permissions, write is now forbidden";
46+
47+
// Check that the file can not be removed
48+
try {
49+
boost::filesystem::remove(file.GetPath());
50+
FAIL() << "Expected exception when removing file";
51+
} catch (const std::exception& ex) {
52+
LOG_DEBUG() << "Confirmed file cannot be removed";
53+
}
54+
55+
// We don't expect that ~TempFile to throw an exception
56+
}
57+
58+
// Check that log with error from ~TempFile exists
59+
EXPECT_THAT(GetLogCapture().Filter("fs::~TempFile failed with exception:"), testing::SizeIs(1));
60+
61+
// Rollback directory permissions
62+
boost::filesystem::permissions(
63+
parent_dir,
64+
boost::filesystem::perms::owner_read | boost::filesystem::perms::owner_write |
65+
boost::filesystem::perms::owner_exe | boost::filesystem::perms::group_read |
66+
boost::filesystem::perms::group_write | boost::filesystem::perms::group_exe |
67+
boost::filesystem::perms::others_read | boost::filesystem::perms::others_write |
68+
boost::filesystem::perms::others_exe
69+
);
70+
LOG_DEBUG() << "Changed directory permissions, write is now available";
71+
}
72+
ASSERT_FALSE(boost::filesystem::exists(parent_dir));
73+
}
74+
75+
USERVER_NAMESPACE_END

‎universal/src/fs/blocking/temp_file.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ TempFile::~TempFile() {
4747
try {
4848
fs::blocking::RemoveSingleFile(path_);
4949
} catch (const std::exception& ex) {
50-
LOG_ERROR() << ex;
50+
LOG_ERROR() << "fs::blocking::~TempFile failed with exception: " << ex;
5151
}
5252
}
5353

‎universal/src/fs/blocking/temp_file_test.cpp‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,13 @@
77

88
#include <userver/fs/blocking/temp_directory.hpp>
99
#include <userver/fs/blocking/write.hpp>
10+
#include <userver/logging/log.hpp>
11+
#include <userver/utest/log_capture_fixture.hpp>
1012

1113
USERVER_NAMESPACE_BEGIN
1214

1315
namespace {
16+
using FsBlockingTempFileWithLog = utest::LogCaptureFixture<>;
1417

1518
bool StartsWith(std::string_view hay, std::string_view needle) { return hay.substr(0, needle.size()) == needle; }
1619

@@ -75,4 +78,59 @@ TEST(TempFile, CustomPath) {
7578
EXPECT_EQ(boost::filesystem::status(root + "/foo").permissions(), boost::filesystem::perms::owner_all);
7679
}
7780

81+
TEST_F(FsBlockingTempFileWithLog, BlockingTempFileDestructorWithException) {
82+
std::string parent_dir;
83+
{
84+
const auto parent = fs::blocking::TempDirectory::Create();
85+
parent_dir = parent.GetPath();
86+
87+
{
88+
// Create temp file in temporaty directory
89+
const auto file = fs::blocking::TempFile::Create(parent_dir, "BlockingTempFileDestructorWithException");
90+
91+
const auto dir_status = boost::filesystem::status(parent_dir);
92+
const auto original_perms = dir_status.permissions();
93+
94+
LOG_DEBUG() << "Original directory permissions: " << static_cast<int>(original_perms);
95+
96+
// Check that the directory has write permissions
97+
ASSERT_TRUE((original_perms & boost::filesystem::perms::owner_write) != boost::filesystem::perms::no_perms);
98+
99+
// Change permissions for the directory
100+
boost::filesystem::permissions(
101+
parent_dir,
102+
boost::filesystem::perms::owner_read | boost::filesystem::perms::owner_exe |
103+
boost::filesystem::perms::group_read | boost::filesystem::perms::group_exe |
104+
boost::filesystem::perms::others_read | boost::filesystem::perms::others_exe
105+
);
106+
LOG_DEBUG() << "Changed directory permissions, write is now forbidden";
107+
108+
// Check that the file can not be removed
109+
try {
110+
boost::filesystem::remove(file.GetPath());
111+
FAIL() << "Expected exception when removing file";
112+
} catch (const std::exception& ex) {
113+
LOG_DEBUG() << "Confirmed file cannot be removed";
114+
}
115+
116+
// We don't expect that ~TempFile to throw an exception
117+
}
118+
119+
// Check that log with error from ~TempFile exists
120+
EXPECT_EQ(GetLogCapture().Filter("fs::blocking::~TempFile failed with exception:").size(), 1);
121+
122+
// Rollback directory permissions
123+
boost::filesystem::permissions(
124+
parent_dir,
125+
boost::filesystem::perms::owner_read | boost::filesystem::perms::owner_write |
126+
boost::filesystem::perms::owner_exe | boost::filesystem::perms::group_read |
127+
boost::filesystem::perms::group_write | boost::filesystem::perms::group_exe |
128+
boost::filesystem::perms::others_read | boost::filesystem::perms::others_write |
129+
boost::filesystem::perms::others_exe
130+
);
131+
LOG_DEBUG() << "Changed directory permissions, write is now available";
132+
}
133+
ASSERT_FALSE(boost::filesystem::exists(parent_dir));
134+
}
135+
78136
USERVER_NAMESPACE_END

0 commit comments

Comments
 (0)