From d9bfbeda9ad2d1f2b16c6abdbdc04995f991ea94 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Fri, 17 Oct 2025 12:10:22 +0300 Subject: [PATCH] lister: Fix race between readdir and stat Sometimes file::list_directory() returns entries without type set. In thase case lister calls file_type() on the entry name to get it. In case the call returns disengated type, the code assumes that some error occurred and resolves into exception. That's not correct. The file_type() method returns disengated type only if the file being inspected is missing (i.e. on ENOENT errno). But this can validly happen if a file is removed bettween readdir and stat. In that case it's not "some error happened", but a enry should be just skipped. In "some error happened", then file_type() would resolve into exceptional future on its own. Signed-off-by: Pavel Emelyanov Closes scylladb/scylladb#26595 --- test/boost/lister_test.cc | 25 +++++++++++++++++++++++++ utils/lister.cc | 22 +++++++++++++++------- utils/lister.hh | 10 ++++++---- 3 files changed, 46 insertions(+), 11 deletions(-) diff --git a/test/boost/lister_test.cc b/test/boost/lister_test.cc index 8125a35416..386e217992 100644 --- a/test/boost/lister_test.cc +++ b/test/boost/lister_test.cc @@ -23,6 +23,7 @@ #include "utils/assert.hh" #include "utils/lister.hh" +#include "utils/error_injection.hh" class expected_exception : public std::exception { public: @@ -186,3 +187,27 @@ SEASTAR_TEST_CASE(test_directory_lister_extra_get) { BOOST_REQUIRE_THROW(co_await dl.get(), seastar::broken_pipe_exception); } + +// regression test of #26314 +SEASTAR_TEST_CASE(test_directory_lister_refresh_type_race) { +#ifdef SCYLLA_ENABLE_ERROR_INJECTION + utils::error_injection_parameters params; + params["unlink_file"] = "unlinkme"; + utils::get_local_injector().enable("lister_refresh_type", false, std::move(params)); + auto tmp = tmpdir(); + co_await tests::touch_file(tmp.path() / "unlinkme"); + co_await tests::touch_file(tmp.path() / "sentinel"); + std::map> files; + co_await lister::scan_dir(tmp.path(), lister::dir_entry_types::full(), [&files] (fs::path dir, directory_entry de) { + BOOST_CHECK(!files.contains(de.name)); + files[de.name] = de.type; + return make_ready_future<>(); + }); + BOOST_CHECK(!files.contains("unlinkme")); + BOOST_CHECK(files.contains("sentinel")); + BOOST_CHECK(files.at("sentinel").has_value()); +#else + fmt::print("Skipping test as it depends on error injection. Please run in mode where it's enabled (debug,dev).\n"); + co_return; +#endif +} diff --git a/utils/lister.cc b/utils/lister.cc index 123af89a47..dfd1d23344 100644 --- a/utils/lister.cc +++ b/utils/lister.cc @@ -7,6 +7,7 @@ #include "utils/assert.hh" #include "utils/lister.hh" #include "utils/checked-file-impl.hh" +#include "utils/error_injection.hh" static seastar::logger llogger("lister"); @@ -20,7 +21,11 @@ lister::lister(file f, dir_entry_types type, walker_type walker, filter_type fil , _show_hidden(do_show_hidden) {} future<> lister::visit(directory_entry de) { - return guarantee_type(std::move(de)).then([this] (directory_entry de) { + return refresh_type(std::move(de)).then([this] (directory_entry de) { + // The entry was removed between readdir and stat, just skip it + if (!de.type.has_value()) { + return make_ready_future<>(); + } // Hide all synthetic directories and hidden files if not requested to show them. if ((_expected_type && !_expected_type.contains(*(de.type))) || (!_show_hidden && de.name[0] == '.')) { return make_ready_future<>(); @@ -41,16 +46,19 @@ future<> lister::done() { }); } -future lister::guarantee_type(directory_entry de) { +future lister::refresh_type(directory_entry de) { + if (utils::get_local_injector().enter("lister_refresh_type")) { + de.type.reset(); + if (de.name == *utils::get_local_injector().inject_parameter("lister_refresh_type", "unlink_file")) { + fs::remove((_dir / de.name).native()); + } + } + if (de.type) { return make_ready_future(std::move(de)); } else { auto f = file_type((_dir / de.name.c_str()).native(), follow_symlink::no); - return f.then([dir = _dir, de = std::move(de)] (std::optional t) mutable { - // If some FS error occurs - return an exceptional future - if (!t) { - return make_exception_future(std::runtime_error(fmt::format("Failed to get {} type.", (dir / de.name.c_str()).native()))); - } + return f.then([de = std::move(de)] (std::optional t) mutable { de.type = t; return make_ready_future(std::move(de)); }); diff --git a/utils/lister.hh b/utils/lister.hh index 83750a1c87..a4028ebf97 100644 --- a/utils/lister.hh +++ b/utils/lister.hh @@ -122,20 +122,22 @@ private: future<> visit(directory_entry de); /** - * Validates that the input parameter has its "type" optional field engaged. + * Try to get entry "type" if it's missing * * This helper method is called before further processing the @param de in order - * to ensure that its "type" field is engaged. + * to have its "type" field engaged. * * If it is engaged - returns the input value as is. * If "type" isn't engaged - calls the file_type() for file represented by @param de and sets - * "type" field of @param de to the returned value and then returns @param de. + * "type" field of @param de to the returned value and then returns @param de. The "type" may + * still be disengaged after it, meaning that the corresponding file is now missing (because it + * was removed or renamed). * * @param de entry to check and return * @return a future that resolves with the @param de with the engaged de.type field or an * exceptional future with std::system_error exception if type of the file represented by @param de may not be retrieved. */ - future guarantee_type(directory_entry de); + future refresh_type(directory_entry de); }; class abstract_lister {