From 4e0ad7442e99a85da0b58d0c5cac08e5c75fa4c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=B0=B4=E8=8A=9D?= Date: Sat, 25 Jul 2026 11:20:13 +0800 Subject: [PATCH] rpc: fix OOO engine stack context UAF (#1291) A timed-out caller could return and destroy its stack OutOfOrderContext while the receiver was still collecting it after a yield in do_collect(). Add m_collecting handshake: the receiver marks the context in flight; timed-out callers, the do_issue() failure path and the wait_completion() entry window wait for the hand-back. notify_all() wakes both waiting callers and shutdown(). The !th branch becomes a legal timed-out state (continue); error_thread_become_NULL replaced by real-path tests. --- rpc/out-of-order-execution.cpp | 67 ++++++-- rpc/out-of-order-execution.h | 6 +- rpc/test/CMakeLists.txt | 1 + rpc/test/test-ooo-timeout.cpp | 288 +++++++++++++++++++++++++++++++++ rpc/test/test-ooo.cpp | 43 ----- 5 files changed, 350 insertions(+), 55 deletions(-) create mode 100644 rpc/test/test-ooo-timeout.cpp diff --git a/rpc/out-of-order-execution.cpp b/rpc/out-of-order-execution.cpp index 4c7c9d90d..78f51012d 100644 --- a/rpc/out-of-order-execution.cpp +++ b/rpc/out-of-order-execution.cpp @@ -33,6 +33,9 @@ namespace rpc { uint64_t m_issuing = 0; uint64_t m_tag = 0; bool m_running = true; + // The context currently being collected by the receiver (under m_mutex_r). + // A timed-out caller must not destroy its stack context until this is cleared. + OutOfOrderContext* m_collecting = nullptr; // rlock used as both reader lock and wait notifier. // add yield in lock will break the assuption that threads @@ -58,6 +61,23 @@ namespace rpc { int get_queue_count() { return m_map.size(); } + // Wait until the receiver hands back the context that it has taken out + // of the map for collecting (see `m_collecting`), so that the caller's + // stack frame hosting the context can be safely destroyed afterwards. + // Precondition: args.phaselock is held by the caller. The receiver + // clears m_collecting under the same lock, and cond wait releases the + // lock atomically, so the check-then-wait cannot lose the wakeup. + // Returns true if the receiver was collecting args; when it returns, + // the hand-back is done and args.phase is COLLECTED. + bool wait_if_collecting(OutOfOrderContext& args) { + if (m_collecting != &args) + return false; + args.th = nullptr; // tell the receiver not to interrupt us + do { + m_cond_collected.wait(args.phaselock); + } while (m_collecting == &args); + return true; + } int issue_operation(OutOfOrderContext& args) //firing issue { SCOPED_LOCK(m_mutex_w); @@ -93,9 +113,18 @@ namespace rpc { int ret2 = args.do_issue(&args); if (ret2 < 0) { - SCOPED_LOCK(m_mutex_map); - m_map.erase(args.tag); - m_cond_collected.notify_one(); + { + SCOPED_LOCK(m_mutex_map); + m_map.erase(args.tag); + m_cond_collected.notify_one(); + } + { + // The receiver may have already taken &args out of the map + // while do_issue() yielded, and may still dereference it + // after yielding in do_collect(). + SCOPED_LOCK(args.phaselock); + wait_if_collecting(args); + } LOG_ERROR_RETURN(0, -1, "failed to do_issue()"); } { @@ -112,13 +141,19 @@ namespace rpc { }; int wait_completion(OutOfOrderContext& args) //recieving work { + bool found; { // check if context issued SCOPED_LOCK(m_mutex_map); - if (m_map.find(args.tag) == m_map.end()) { - LOG_ERROR_RETURN(EINVAL, -1, - "context not found in map"); - } + found = m_map.find(args.tag) != m_map.end(); + } + if (!found) { + // The receiver may have just taken this context out of the map + // and could still be collecting it after a yield in do_collect(). + SCOPED_LOCK(args.phaselock); + if (wait_if_collecting(args) && args.phase == OooPhase::COLLECTED) + return args.ret; + LOG_ERROR_RETURN(EINVAL, -1, "context not found in map"); } DEFER(m_wait.notify_one()); { @@ -156,6 +191,14 @@ namespace rpc { m_map.erase(args.tag); m_cond_collected.notify_one(); } + if (wait_if_collecting(args)) { + // The receiver had already taken our pointer out + // of the map and may dereference it after yielding + // in do_collect(). Now that the result has actually + // been collected, return it as a success, consistent + // with the COLLECTED check above. + return args.ret; + } LOG_ERROR_RETURN(ETIMEDOUT, -1, "waiting for completion timeout"); } break; @@ -198,6 +241,7 @@ namespace rpc { } targ = it->second; m_map.erase(it); + m_collecting = targ; } // collect with mutex_r @@ -209,7 +253,10 @@ namespace rpc { SCOPED_LOCK(targ->phaselock); th = targ->th; targ->phase = OooPhase::COLLECTED; + m_collecting = nullptr; } + // both timed-out callers and shutdown() may be waiting + m_cond_collected.notify_all(); if (o_tag == args.tag) { if (th != CURRENT) { LOG_ERROR_RETURN(EINVAL, -1, "args tag ` not belong to current thread `", VALUE(args.tag), VALUE(CURRENT)); @@ -218,8 +265,10 @@ namespace rpc { // collect it } if (!th) - // issued but requesting thread just failed in completion when waiting - LOG_ERROR_RETURN(ENOENT, -2, "response recvd, but requesting thread is NULL!"); + // a timed-out caller cleared `th` in wait_if_collecting() to + // tell us not to interrupt it; it is waiting for the hand-back + // above, so keep looping to receive our own result + continue; thread_interrupt(th, EINTR); // other threads' response, resume him } } diff --git a/rpc/out-of-order-execution.h b/rpc/out-of-order-execution.h index 5eca97796..a97382072 100644 --- a/rpc/out-of-order-execution.h +++ b/rpc/out-of-order-execution.h @@ -123,9 +123,9 @@ namespace rpc { // Wait for the completion of the operation. // returns 0 for success, negative for failures - // if returns -2 and errno == ENOENT, there is a completed - // operation but there is no caller in the registry to - // collect the result, so users have to fix it up. + // if returns -2 and errno == ENOENT, a response was received + // whose tag is not in the registry (never issued, or its caller + // already timed out and deregistered), and should be dropped. // Arguments: engine, do_issue, [tag, flag_tag_valid], do_completion extern "C" int ooo_wait_completion(OutOfOrderContext& args); diff --git a/rpc/test/CMakeLists.txt b/rpc/test/CMakeLists.txt index 75bd3637c..bc47d98d7 100644 --- a/rpc/test/CMakeLists.txt +++ b/rpc/test/CMakeLists.txt @@ -1,3 +1,4 @@ photon_add_test(test-rpc test.cpp) photon_add_test(test-ooo test-ooo.cpp) +photon_add_test(test-ooo-timeout test-ooo-timeout.cpp) photon_add_test(test-rpc-message test-rpc-message.cpp) diff --git a/rpc/test/test-ooo-timeout.cpp b/rpc/test/test-ooo-timeout.cpp new file mode 100644 index 000000000..197d60d14 --- /dev/null +++ b/rpc/test/test-ooo-timeout.cpp @@ -0,0 +1,288 @@ +/* +Copyright 2022 The Photon Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// Regression tests for issue #1291: use-after-free of the caller's stack +// OutOfOrderContext when it times out while the receiver is collecting it +// (i.e. the receiver yielded inside do_collect() after taking the pointer +// out of the map). + +#include +#include +#include "../../test/gtest.h" +#include "../out-of-order-execution.h" +#include +#include +#include +#include + +using namespace photon; +using namespace photon::rpc; + +// A fake completion source that simulates out-of-order completion without a +// real network: completed tags are pushed into a shared FIFO in an arbitrary +// order (by do_issue or by the test body), and do_completion pops one, which +// is not necessarily the tag of the calling context. +struct FakeSource { + std::queue done; + int issue(OutOfOrderContext* args) { + done.push(args->tag); + return 0; + } + int complete(OutOfOrderContext* args) { + while (done.empty()) + thread_yield(); + args->tag = done.front(); + done.pop(); + return 0; + } +}; + +// A do_collect that yields for a while, opening the window in which the +// receiver holds a bare context pointer across a coroutine switch. +struct SlowCollect { + uint64_t sleep_us; + int collected = 0; + int collect(OutOfOrderContext*) { + thread_usleep(sleep_us); + collected++; + return 0; + } +}; + +// A do_issue that pushes its tag first (so the receiver can see the +// response) and reports failure only after a yield, by which time the +// receiver may have taken the context out of the map into do_collect(). +struct FailAfterCollectIssue { + FakeSource* src; + uint64_t sleep_us; + int issue(OutOfOrderContext* args) { + src->done.push(args->tag); + thread_usleep(sleep_us); + return -1; + } +}; + +static int null_op(void*, OutOfOrderContext*) { + return 0; +} + +// The caller times out while the receiver is yielding inside do_collect() +// with the caller's context pointer in hand. The caller must wait for the +// hand-back instead of destroying its stack frame, and then return the +// collected result as a success. +TEST(OutOfOrderTimeout, timeout_during_collect_yield) { + auto engine = new_ooo_execution_engine(); + DEFER(delete_ooo_execution_engine(engine)); + FakeSource src; + + // caller A has no timeout; it becomes the receiver and blocks in + // complete() until a tag is pushed + OutOfOrderContext a; + a.engine = engine; + a.do_issue.bind(nullptr, &null_op); + a.do_completion.bind(&src, &FakeSource::complete); + a.do_collect.bind(nullptr, &null_op); + int a_ret = -100; + auto tha = thread_enable_join(thread_create11([&] { + ASSERT_EQ(0, ooo_issue_operation(a)); + a_ret = ooo_wait_completion(a); + })); + thread_yield(); // let A issue and become the receiver + // do_completion overwrites a.tag with whatever it pops, so save the + // original tag now for completing A's own operation later + auto a_tag = a.tag; + + // caller B times out (10ms) while its result is being collected (100ms) + SlowCollect sc{100 * 1000}; + OutOfOrderContext b; + b.engine = engine; + b.do_issue.bind(nullptr, &null_op); + b.do_completion.bind(&src, &FakeSource::complete); + b.do_collect.bind(&sc, &SlowCollect::collect); + b.timeout = Timeout(10 * 1000); + int b_ret = -100; + auto thb = thread_enable_join(thread_create11([&] { + ASSERT_EQ(0, ooo_issue_operation(b)); + b_ret = ooo_wait_completion(b); + })); + thread_yield(); // let B issue and enter the waiting queue + + // complete B's operation: the receiver A takes B's context out of the + // map and yields in do_collect(); B's timeout fires inside this window + src.done.push(b.tag); + thread_join(thb); + + // B waited for the hand-back and returned the collected result + EXPECT_EQ(0, b_ret); + EXPECT_EQ(1, sc.collected); + EXPECT_EQ((int)OooPhase::COLLECTED, (int)b.phase); + // B cleared `th` so that the receiver skips thread_interrupt() + EXPECT_EQ(nullptr, b.th); + + // the receiver must have taken the `!th -> continue` branch and kept + // looping for its own result, instead of aborting with -2/ENOENT + src.done.push(a_tag); + thread_join(tha); + EXPECT_EQ(0, a_ret); + EXPECT_EQ(0, ooo_get_queue_count(engine)); +} + +// The caller yields between issue and wait_completion, and the receiver +// takes its context out of the map in between. wait_completion() then does +// not find the tag in the map, but must not return EINVAL while the +// receiver is still collecting the context. +TEST(OutOfOrderTimeout, collected_between_issue_and_wait) { + auto engine = new_ooo_execution_engine(); + DEFER(delete_ooo_execution_engine(engine)); + FakeSource src; + + OutOfOrderContext a; + a.engine = engine; + a.do_issue.bind(nullptr, &null_op); + a.do_completion.bind(&src, &FakeSource::complete); + a.do_collect.bind(nullptr, &null_op); + int a_ret = -100; + auto tha = thread_enable_join(thread_create11([&] { + ASSERT_EQ(0, ooo_issue_operation(a)); + a_ret = ooo_wait_completion(a); + })); + thread_yield(); // let A issue and become the receiver + // do_completion overwrites a.tag with whatever it pops, so save the + // original tag now for completing A's own operation later + auto a_tag = a.tag; + + SlowCollect sc{50 * 1000}; + OutOfOrderContext b; + b.engine = engine; + b.do_issue.bind(&src, &FakeSource::issue); // completes immediately + b.do_completion.bind(&src, &FakeSource::complete); + b.do_collect.bind(&sc, &SlowCollect::collect); + int b_ret = -100; + auto thb = thread_enable_join(thread_create11([&] { + ASSERT_EQ(0, ooo_issue_operation(b)); + // yield into the window where the receiver has erased b's tag from + // the map and is sleeping in do_collect() + thread_usleep(10 * 1000); + b_ret = ooo_wait_completion(b); + })); + thread_join(thb); + + EXPECT_EQ(0, b_ret); + EXPECT_EQ(1, sc.collected); + EXPECT_EQ((int)OooPhase::COLLECTED, (int)b.phase); + EXPECT_EQ(nullptr, b.th); + + src.done.push(a_tag); + thread_join(tha); + EXPECT_EQ(0, a_ret); + EXPECT_EQ(0, ooo_get_queue_count(engine)); +} + +// do_issue() fails after the receiver has already taken the context out of +// the map (the tag was pushed before the failure, and do_issue yielded). +// The failing caller must wait for the hand-back in the do_issue failure +// path before destroying its stack frame. +TEST(OutOfOrderTimeout, issue_failure_during_collect) { + log_output = log_output_null; // silence the expected do_issue error + DEFER(log_output = log_output_stdout); + auto engine = new_ooo_execution_engine(); + DEFER(delete_ooo_execution_engine(engine)); + FakeSource src; + + OutOfOrderContext a; + a.engine = engine; + a.do_issue.bind(nullptr, &null_op); + a.do_completion.bind(&src, &FakeSource::complete); + a.do_collect.bind(nullptr, &null_op); + int a_ret = -100; + auto tha = thread_enable_join(thread_create11([&] { + ASSERT_EQ(0, ooo_issue_operation(a)); + a_ret = ooo_wait_completion(a); + })); + thread_yield(); // let A issue and become the receiver + // do_completion overwrites a.tag with whatever it pops, so save the + // original tag now for completing A's own operation later + auto a_tag = a.tag; + + // B's do_issue pushes b.tag and yields for 10ms before failing; the + // receiver A pops the tag, takes B's context out of the map and sleeps + // 100ms in do_collect(), so do_issue reports failure inside the window + SlowCollect sc{100 * 1000}; + FailAfterCollectIssue fi{&src, 10 * 1000}; + OutOfOrderContext b; + b.engine = engine; + b.do_issue.bind(&fi, &FailAfterCollectIssue::issue); + b.do_completion.bind(&src, &FakeSource::complete); + b.do_collect.bind(&sc, &SlowCollect::collect); + int b_ret = -100; + auto thb = thread_enable_join(thread_create11([&] { + b_ret = ooo_issue_operation(b); + })); + thread_join(thb); + + // issue failed, but only after the hand-back was done + EXPECT_EQ(-1, b_ret); + EXPECT_EQ(1, sc.collected); + EXPECT_EQ((int)OooPhase::COLLECTED, (int)b.phase); + EXPECT_EQ(nullptr, b.th); + + src.done.push(a_tag); + thread_join(tha); + EXPECT_EQ(0, a_ret); + EXPECT_EQ(0, ooo_get_queue_count(engine)); +} + +// N concurrent callers with tiny timeouts against slow, rotating receivers. +// Callers may be collected in time (0), time out before being collected +// (-1/ETIMEDOUT), or abort as a receiver on a stale tag (-2/ENOENT); in all +// cases there must be no crash and the map must drain completely. +TEST(OutOfOrderTimeout, concurrent_timeout_stress) { + log_output = log_output_null; // silence expected timeout/drop errors + DEFER(log_output = log_output_stdout); + auto engine = new_ooo_execution_engine(); + DEFER(delete_ooo_execution_engine(engine)); + FakeSource src; + SlowCollect sc{5 * 1000}; + + constexpr int N = 50; + int done = 0; + std::vector ths; + for (int i = 0; i < N; ++i) { + ths.push_back(thread_enable_join(thread_create11([&] { + OutOfOrderContext args; + args.engine = engine; + args.do_issue.bind(&src, &FakeSource::issue); + args.do_completion.bind(&src, &FakeSource::complete); + args.do_collect.bind(&sc, &SlowCollect::collect); + args.timeout = Timeout(1000); + int ret = ooo_issue_wait(args); + EXPECT_TRUE(ret == 0 || ret == -1 || ret == -2); + done++; + }))); + } + for (auto th : ths) + thread_join(th); + EXPECT_EQ(N, done); + EXPECT_EQ(0, ooo_get_queue_count(engine)); +} + +int main(int argc, char** argv) { + ::testing::InitGoogleTest(&argc, argv); + if (photon::init(photon::INIT_EVENT_DEFAULT, photon::INIT_IO_NONE)) + return -1; + DEFER(photon::fini()); + return RUN_ALL_TESTS(); +} diff --git a/rpc/test/test-ooo.cpp b/rpc/test/test-ooo.cpp index 56e6e5e3b..d14fbf4fa 100644 --- a/rpc/test/test-ooo.cpp +++ b/rpc/test/test-ooo.cpp @@ -357,48 +357,6 @@ TEST(OutOfOrder, error_change_arg) { log_output = log_output_stdout; } -int error_change_engine_complete(void*, OutOfOrderContext* args) { - OooEngine* engine = (OooEngine*)(args->engine); - if (args->tag == 1) { - thread_usleep(1000); - args->tag = 2; - } else { - args->tag = 1; - } - engine->m_map[args->tag]->th = nullptr; - return 0; -} - -void error_change_engine_issuewait(OutOfOrder_Execution_Engine * engine) { - OutOfOrderContext args; - args.engine = engine; - args.do_issue.bind(nullptr, null_op); - args.do_completion.bind(nullptr, error_change_engine_complete); - args.do_collect.bind(nullptr, null_op); - int ret = ooo_issue_wait(args); - EXPECT_EQ(-1, ret); - EXPECT_EQ(EINVAL, errno); -} - -TEST(OutOfOrder, error_thread_become_NULL) { - log_output = log_output_null; - OutOfOrder_Execution_Engine * engine = new_ooo_execution_engine(); - DEFER({ - wait_for_completion(); - // delete_ooo_execution_engine(engine); - }); - OutOfOrderContext args; - args.engine = engine; - args.do_issue.bind(nullptr, null_op); - args.do_completion.bind(nullptr, error_change_engine_complete); - thread_create11(error_change_engine_issuewait, engine); - int ret = ooo_issue_wait(args); - EXPECT_EQ(-2, ret); - EXPECT_EQ(ENOENT, errno); - wait_for_completion(); - log_output = log_output_stdout; -} - void run_all_tests(uint32_t i) { #define RUN_TEST(A, B) LOG_DEBUG("vCPU #", i, ": "#A":"#B); A##_##B##_Test().TestBody(); RUN_TEST(OutOfOrder, Execution); @@ -409,7 +367,6 @@ void run_all_tests(uint32_t i) { RUN_TEST(OutOfOrder, error_process); RUN_TEST(OutOfOrder, error_same_tag); RUN_TEST(OutOfOrder, error_change_arg); - RUN_TEST(OutOfOrder, error_thread_become_NULL); wait_for_completion(i); #undef RUN_TEST }