From: Alexander Indenbaum Date: Mon, 23 Feb 2026 07:04:30 +0000 (+0200) Subject: crimson: fix stack-use-after-return in ConfigProxy::parse_argv X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=3719aec189d3270e16ba1bf813f1a131f3b4efc4;p=ceph-ci.git crimson: fix stack-use-after-return in ConfigProxy::parse_argv ConfigProxy::parse_argv used to take std::vector& and capture it by reference in a lambda passed to do_change(), which invokes the lambda on another shard via invoke_on. By the time the target shard ran the lambda, the caller had returned and the referenced vector was destroyed. Fix: take std::vector by value and move it into the closure, so the data outlives the call. Convert to vector only on the target shard. - update callers to pass vector - add unittest-seastar-config-parse-argv-uaf to demonstrate the pattern Signed-off-by: Alexander Indenbaum --- diff --git a/src/crimson/common/config_proxy.h b/src/crimson/common/config_proxy.h index ee4595ad3e2..c19cfd201dd 100644 --- a/src/crimson/common/config_proxy.h +++ b/src/crimson/common/config_proxy.h @@ -192,17 +192,13 @@ public: } void show_config(ceph::Formatter* f) const; - seastar::future<> parse_argv(std::vector& argv) { - // we could pass whatever is unparsed to seastar, but seastar::app_template - // is used for driving the seastar application, and - // crimson::common::ConfigProxy is not available until seastar engine is up - // and running, so we have to feed the command line args to app_template - // first, then pass them to ConfigProxy. - return do_change([&argv, this](ConfigValues& values) { - get_config().parse_argv(values, - obs_mgr, - argv, - CONF_CMDLINE); + seastar::future<> parse_argv(std::vector args) { + // Take args by value so the lambda can safely be invoked on another shard + return do_change([args = std::move(args), this](ConfigValues& values) { + std::vector argv; + argv.reserve(args.size()); + for (const auto& s : args) argv.push_back(s.c_str()); + get_config().parse_argv(values, obs_mgr, argv, CONF_CMDLINE); }); } diff --git a/src/crimson/osd/main.cc b/src/crimson/osd/main.cc index 461a2652ecc..a65905fa40a 100644 --- a/src/crimson/osd/main.cc +++ b/src/crimson/osd/main.cc @@ -98,7 +98,7 @@ int main(int argc, const char* argv[]) auto &early_config = early_config_result.value(); auto seastar_n_early_args = early_config.get_early_args(); - auto config_proxy_args = early_config.get_ceph_args(); + auto config_proxy_args = early_config.ceph_args; seastar::app_template::config app_cfg; app_cfg.name = "Crimson"; diff --git a/src/crimson/osd/main_config_bootstrap_helpers.cc b/src/crimson/osd/main_config_bootstrap_helpers.cc index 090c54ee495..adf7d630b13 100644 --- a/src/crimson/osd/main_config_bootstrap_helpers.cc +++ b/src/crimson/osd/main_config_bootstrap_helpers.cc @@ -165,7 +165,10 @@ _get_early_config(int argc, const char *argv[]) auto stop_perf_coll = seastar::deferred_stop(sharded_perf_coll()); local_conf().parse_env().get(); - local_conf().parse_argv(early_args).get(); + { + std::vector args_copy(early_args.begin(), early_args.end()); + local_conf().parse_argv(std::move(args_copy)).get(); + } local_conf().parse_config_files(ret.conf_file_list).get(); if (local_conf()->no_mon_config) { diff --git a/src/crimson/tools/store_bench/store-bench.cc b/src/crimson/tools/store_bench/store-bench.cc index 18fe9af9d5f..4811d859e85 100644 --- a/src/crimson/tools/store_bench/store-bench.cc +++ b/src/crimson/tools/store_bench/store-bench.cc @@ -883,11 +883,7 @@ int main(int argc, char **argv) { co_await crimson::common::local_conf().start(); { - std::vector cav; - std::transform( - std::begin(unrecognized_options), std::end(unrecognized_options), - std::back_inserter(cav), [](auto &s) { return s.c_str(); }); - co_await crimson::common::local_conf().parse_argv(cav); + co_await crimson::common::local_conf().parse_argv(unrecognized_options); } auto store = crimson::os::FuturizedStore::create( diff --git a/src/test/crimson/CMakeLists.txt b/src/test/crimson/CMakeLists.txt index 48484410a06..43028e8c92a 100644 --- a/src/test/crimson/CMakeLists.txt +++ b/src/test/crimson/CMakeLists.txt @@ -55,6 +55,12 @@ add_ceph_unittest(unittest-seastar-config --memory 256M --smp 4) target_link_libraries(unittest-seastar-config crimson) +add_executable(unittest-seastar-config-parse-argv-uaf + test_config_parse_argv_uaf.cc) +add_ceph_unittest(unittest-seastar-config-parse-argv-uaf + --memory 256M --smp 2) +target_link_libraries(unittest-seastar-config-parse-argv-uaf crimson) + add_executable(unittest-seastar-monc test_monc.cc) target_link_libraries(unittest-seastar-monc crimson) diff --git a/src/test/crimson/test_config_parse_argv_uaf.cc b/src/test/crimson/test_config_parse_argv_uaf.cc new file mode 100644 index 00000000000..8ac24601aa7 --- /dev/null +++ b/src/test/crimson/test_config_parse_argv_uaf.cc @@ -0,0 +1,93 @@ +// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:nil -*- +// vim: ts=8 sw=2 expandtab +// +// Minimal unit test demonstrating stack-use-after-return when passing +// a reference to a local variable into a lambda that runs on another shard +// via invoke_on to replicate the pattern that ConfigProxy::parse_argv had before +// the fix (taking vector& and capturing by reference in do_change) +// +// The old path captures [&argv]; the lambda runs on shard 1 after the caller +// on shard 0 has returned, so argv (on shard 0's stack) is destroyed +// +// Run: ./bin/unittest-seastar-config-parse-argv-uaf --smp 2 +// Without ASan stack-use-after-return: may crash (SIGABRT) due to UAF +// With ASAN_OPTIONS=detect_stack_use_after_return=1: may detect the UAF +// +// Build: ninja unittest-seastar-config-parse-argv-uaf +// + +#include +#include +#include +#include "test/crimson/ctest_utils.h" + +using namespace seastar; + +struct TargetService {}; + +static sharded* g_svc = nullptr; + +// Mimics the old ConfigProxy::parse_argv pattern: takes reference, captures +// in lambda sent to another shard. When lambda runs, caller's stack is gone. +static future<> buggy_parse_argv_like(std::vector& argv) { + return g_svc->invoke_on(1, [&argv](TargetService&) { + volatile size_t n = argv.size(); // use-after-return: argv was on caller's stack + (void)n; + }); +} + +// New approach: take by value, move into closure so data outlives the call +static future<> correct_parse_argv_like(std::vector args) { + return g_svc->invoke_on(1, [args = std::move(args)](TargetService&) { + volatile size_t n = args.size(); // safe: args is captured by value + (void)n; + }); +} + +static future<> test_uaf() +{ + if (smp::count < 2) { + std::cerr << "Skip: need --smp 2 to demonstrate cross-shard use-after-return" << std::endl; + return make_ready_future<>(); + } + + // Run the old call from shard 0. The lambda is sent to shard 1. + // By the time shard 1 runs it, the caller on shard 0 has returned and + // 'argv' (on shard 0's stack) is gone. + return smp::submit_to(0, [] { + std::vector argv = {"--foo=1", "--bar=2"}; + return buggy_parse_argv_like(argv); + }); +} + +static future<> test_correct() +{ + if (smp::count < 2) { + return make_ready_future<>(); + } + std::vector args = {"--foo=1", "--bar=2"}; + return correct_parse_argv_like(std::move(args)); +} + +int main(int argc, char** argv) +{ + app_template app{get_smp_opts_from_ctest()}; + static sharded svc; // static so it outlives the async chain + return app.run(argc, argv, [&] { + return svc.start().then([] { + g_svc = &svc; + return test_correct(); + }).then([] { + return test_uaf(); + }).finally([] { + g_svc = nullptr; + return svc.stop(); + }).then([] { + std::cout << "Done. With ASAN_OPTIONS=detect_stack_use_after_return=1, " + << "the buggy path may trigger use-after-return detection." << std::endl; + }).handle_exception([](auto e) { + std::cerr << "Test failed: " << e << std::endl; + return make_exception_future<>(e); + }); + }); +}