From 5e11458bc6274be3a8fa75940cc6b8d19a69891a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1t=C3=A9=20Szab=C3=B3?= Date: Mon, 6 Oct 2025 09:47:40 -0400 Subject: [PATCH] Fix stack overflow on long AwaitAllWaitHandle chains When an `AwaitAllWaitHandle` is unblocked, it checks whether all its children are finished, and unblocks its parent if so. This can lead to a stack overflow if an extremely long chain of `AwaitAllWaitHandle`s has formed. This is very easy to trigger e.g. by using a HSL `Semaphore` with a slow task: ```hack async function bar(): Awaitable { $s = new \HH\Lib\Async\Semaphore(10, async (bool $should_wait) ==> { if ($should_wait) { await Asio\usleep(100000 * 100000); } else { await Asio\later(); } }); concurrent { await $s->waitForAsync(true); await async { for ($i = 0; $i < 1000000; $i++) { await $s->waitForAsync(false); } }; } } <<__EntryPoint>> function mymain(): void { Asio\join(bar()); } ``` When this runs, we either crash in `AsioBlockableChain::unblock` as we recursively try to unblock each handle in the chain, or in the native destructor for `AwaitAllWaitHandle` as we recursively try to free the entire chain. `ConcurrentWaitHandle` is implemented in a similar vein and may thus also suffer from the same issue. So: * When freeing `AwaitAllWaitHandle`s or `ConcurrentWaitHandle`s, iteratively decrement the refcount for any children that are themselves of the same type. and free their backing memory in a separate loop instead of deeply recursing via `decRefObj()`. Handle other children as before. * Avoid recursion in `AsioBlockableChain::unblock` via a worklist. Open questions: * Should we also handle a chain that consists of mixed `AwaitAllWaitHandle`s and `ConcurrentWaitHandle`s? I haven't been able to create a reproducer that'd create such a chain. * The iterative `AsioBlockableChain::unblock` implementation now also leaves `m_lastParent` untouched, which is also how it was before `f80acc289feb1a76029d08447c993138db397a29`. (D3053017). If I'm reading the code correctly, this should not have an effect for most `parentChain` calls because those were operating on a copied value to begin with, but I'm not very confident about this. --- hphp/runtime/ext/asio/asio-blockable.cpp | 52 +++++++++++-------- .../ext/asio/ext_await-all-wait-handle.cpp | 14 ++--- .../ext/asio/ext_await-all-wait-handle.h | 37 ++++++++++--- .../ext/asio/ext_concurrent-wait-handle.cpp | 14 ++--- .../ext/asio/ext_concurrent-wait-handle.h | 37 ++++++++++--- .../ext/asio/ext_condition-wait-handle.cpp | 4 +- .../ext/asio/ext_condition-wait-handle.h | 4 +- .../asio/ext_priority-bridge-wait-handle.cpp | 4 +- .../asio/ext_priority-bridge-wait-handle.h | 4 +- hphp/test/slow/async/long_awaitall_chain.php | 29 +++++++++++ .../slow/async/long_awaitall_chain.php.expect | 1 + 11 files changed, 141 insertions(+), 59 deletions(-) create mode 100644 hphp/test/slow/async/long_awaitall_chain.php create mode 100644 hphp/test/slow/async/long_awaitall_chain.php.expect diff --git a/hphp/runtime/ext/asio/asio-blockable.cpp b/hphp/runtime/ext/asio/asio-blockable.cpp index ad25c9ecec1619..7c885773889f5c 100644 --- a/hphp/runtime/ext/asio/asio-blockable.cpp +++ b/hphp/runtime/ext/asio/asio-blockable.cpp @@ -166,29 +166,35 @@ c_WaitableWaitHandle* AsioBlockable::getWaitHandle() const { } void AsioBlockableChain::unblock() { - while (auto cur = m_lastParent) { - m_lastParent = cur->getPrevParent(); - cur->updatePrevParent(nullptr); - // the onUnblocked handler may free cur - switch (cur->getKind()) { - case Kind::AsyncFunctionWaitHandleNode: - getAsyncFunctionWaitHandleNode(cur)->onUnblocked(); - break; - case Kind::AsyncGeneratorWaitHandle: - getAsyncGeneratorWaitHandle(cur)->onUnblocked(); - break; - case Kind::AwaitAllWaitHandleNode: - getAwaitAllWaitHandleNode(cur)->onUnblocked(); - break; - case Kind::ConcurrentWaitHandleNode: - getConcurrentWaitHandleNode(cur)->onUnblocked(); - break; - case Kind::ConditionWaitHandle: - getConditionWaitHandle(cur)->onUnblocked(); - break; - case Kind::PriorityBridgeWaitHandle: - getPriorityBridgeWaitHandle(cur)->onUnblocked(); - break; + std::vector worklist = { *this }; + while (!worklist.empty()) { + auto const lastParent = worklist.back().m_lastParent; + worklist.pop_back(); + + for (AsioBlockable* cur = lastParent, *next; cur; cur = next) { + next = cur->getPrevParent(); + cur->updatePrevParent(nullptr); + // the onUnblocked handler may free cur + switch (cur->getKind()) { + case Kind::AsyncFunctionWaitHandleNode: + getAsyncFunctionWaitHandleNode(cur)->onUnblocked(); + break; + case Kind::AsyncGeneratorWaitHandle: + getAsyncGeneratorWaitHandle(cur)->onUnblocked(); + break; + case Kind::AwaitAllWaitHandleNode: + getAwaitAllWaitHandleNode(cur)->onUnblocked(worklist); + break; + case Kind::ConcurrentWaitHandleNode: + getConcurrentWaitHandleNode(cur)->onUnblocked(worklist); + break; + case Kind::ConditionWaitHandle: + getConditionWaitHandle(cur)->onUnblocked(worklist); + break; + case Kind::PriorityBridgeWaitHandle: + getPriorityBridgeWaitHandle(cur)->onUnblocked(worklist); + break; + } } } } diff --git a/hphp/runtime/ext/asio/ext_await-all-wait-handle.cpp b/hphp/runtime/ext/asio/ext_await-all-wait-handle.cpp index 0de3267122e406..c3534d4a157c66 100644 --- a/hphp/runtime/ext/asio/ext_await-all-wait-handle.cpp +++ b/hphp/runtime/ext/asio/ext_await-all-wait-handle.cpp @@ -149,7 +149,7 @@ void c_AwaitAllWaitHandle::initialize(ContextStateIndex ctxStateIdx) { } } -void c_AwaitAllWaitHandle::onUnblocked(uint32_t idx) { +void c_AwaitAllWaitHandle::onUnblocked(uint32_t idx, std::vector& worklist) { assertx(idx <= m_unfinished); assertx(getState() == STATE_BLOCKED); @@ -166,25 +166,25 @@ void c_AwaitAllWaitHandle::onUnblocked(uint32_t idx) { } catch (const Object& cycle_exception) { assertx(cycle_exception->instanceof(SystemLib::getThrowableClass())); throwable_recompute_backtrace_from_wh(cycle_exception.get(), this); - markAsFailed(cycle_exception); + markAsFailed(cycle_exception, worklist); } return; } } // All children finished. - markAsFinished(); + markAsFinished(worklist); } } -void c_AwaitAllWaitHandle::markAsFinished() { +void c_AwaitAllWaitHandle::markAsFinished(std::vector& worklist) { auto parentChain = getParentChain(); setState(STATE_SUCCEEDED); tvWriteNull(m_resultOrException); - parentChain.unblock(); + worklist.emplace_back(std::move(parentChain)); decRefObj(this); } -void c_AwaitAllWaitHandle::markAsFailed(const Object& exception) { +void c_AwaitAllWaitHandle::markAsFailed(const Object& exception, std::vector& worklist) { for (uint32_t idx = 0; idx < m_cap; idx++) { auto const child = m_children[idx].m_child; if (!child->isFinished()) { @@ -196,7 +196,7 @@ void c_AwaitAllWaitHandle::markAsFailed(const Object& exception) { auto parentChain = getParentChain(); setState(STATE_FAILED); tvWriteObject(exception.get(), &m_resultOrException); - parentChain.unblock(); + worklist.emplace_back(std::move(parentChain)); decRefObj(this); } diff --git a/hphp/runtime/ext/asio/ext_await-all-wait-handle.h b/hphp/runtime/ext/asio/ext_await-all-wait-handle.h index 740b7fb387daf8..43037888fffad2 100644 --- a/hphp/runtime/ext/asio/ext_await-all-wait-handle.h +++ b/hphp/runtime/ext/asio/ext_await-all-wait-handle.h @@ -17,6 +17,8 @@ #pragma once +#include + #include "hphp/runtime/base/type-array.h" #include "hphp/runtime/base/type-object.h" #include "hphp/runtime/ext/asio/ext_waitable-wait-handle.h" @@ -37,9 +39,28 @@ struct c_AwaitAllWaitHandle final : c_WaitableWaitHandle, static void instanceDtor(ObjectData* obj, const Class*) { auto wh = wait_handle(obj); - auto const sz = wh->heapSize(); - wh->~c_AwaitAllWaitHandle(); - tl_heap->objFree(obj, sz); + + std::vector queue = {wh}; + for (std::size_t i = 0; i < queue.size(); i++) { + auto cur = queue[i]; + for (int32_t j = 0; j < cur->m_cap; j++) { + auto cur_child = cur->m_children[j].m_child; + assertx(isFailed() || cur_child->isFinished()); + + if (cur_child->getKind() == Kind::AwaitAll) { + if (cur_child->decReleaseCheck()) { + queue.push_back(cur_child->asAwaitAll()); + } + } else { + decRefObj(cur_child); + } + } + } + + for (auto& cur : queue) { + auto const sz = cur->heapSize(); + tl_heap->objFree(cur, sz); + } } explicit c_AwaitAllWaitHandle(unsigned cap = 0) @@ -79,8 +100,8 @@ struct c_AwaitAllWaitHandle final : c_WaitableWaitHandle, return getChildIdx() == getWaitHandle()->m_unfinished; } - void onUnblocked() { - getWaitHandle()->onUnblocked(getChildIdx()); + void onUnblocked(std::vector& worklist) { + getWaitHandle()->onUnblocked(getChildIdx(), worklist); } AsioBlockable m_blockable; @@ -93,7 +114,7 @@ struct c_AwaitAllWaitHandle final : c_WaitableWaitHandle, } String getName(); - void onUnblocked(uint32_t idx); + void onUnblocked(uint32_t idx, std::vector& worklist); c_WaitableWaitHandle* getChild(); template void forEachChild(T fn); @@ -108,8 +129,8 @@ struct c_AwaitAllWaitHandle final : c_WaitableWaitHandle, static Object Create(Iter iter); static req::ptr Alloc(int32_t cnt); void initialize(ContextStateIndex ctxStateIdx); - void markAsFinished(void); - void markAsFailed(const Object& exception); + void markAsFinished(std::vector& worklist); + void markAsFailed(const Object& exception, std::vector& worklist); void setState(uint8_t state) { setKindState(Kind::AwaitAll, state); } // Construct an AAWH from an array-like without making layout assumptions. diff --git a/hphp/runtime/ext/asio/ext_concurrent-wait-handle.cpp b/hphp/runtime/ext/asio/ext_concurrent-wait-handle.cpp index 8a1a6241135f38..20e09c20c49ea7 100644 --- a/hphp/runtime/ext/asio/ext_concurrent-wait-handle.cpp +++ b/hphp/runtime/ext/asio/ext_concurrent-wait-handle.cpp @@ -101,7 +101,7 @@ void c_ConcurrentWaitHandle::initialize(ContextStateIndex ctxStateIdx) { } } -void c_ConcurrentWaitHandle::onUnblocked(uint32_t idx) { +void c_ConcurrentWaitHandle::onUnblocked(uint32_t idx, std::vector& worklist) { assertx(idx <= m_unfinished); assertx(getState() == STATE_BLOCKED); @@ -118,25 +118,25 @@ void c_ConcurrentWaitHandle::onUnblocked(uint32_t idx) { } catch (const Object& cycle_exception) { assertx(cycle_exception->instanceof(SystemLib::getThrowableClass())); throwable_recompute_backtrace_from_wh(cycle_exception.get(), this); - markAsFailed(cycle_exception); + markAsFailed(cycle_exception, worklist); } return; } } // All children finished. - markAsFinished(); + markAsFinished(worklist); } } -void c_ConcurrentWaitHandle::markAsFinished() { +void c_ConcurrentWaitHandle::markAsFinished(std::vector& worklist) { auto parentChain = getParentChain(); setState(STATE_SUCCEEDED); tvWriteNull(m_resultOrException); - parentChain.unblock(); + worklist.emplace_back(std::move(parentChain)); decRefObj(this); } -void c_ConcurrentWaitHandle::markAsFailed(const Object& exception) { +void c_ConcurrentWaitHandle::markAsFailed(const Object& exception, std::vector& worklist) { for (uint32_t idx = 0; idx < m_cap; idx++) { auto const child = m_children[idx].m_child; if (!child->isFinished()) { @@ -148,7 +148,7 @@ void c_ConcurrentWaitHandle::markAsFailed(const Object& exception) { auto parentChain = getParentChain(); setState(STATE_FAILED); tvWriteObject(exception.get(), &m_resultOrException); - parentChain.unblock(); + worklist.emplace_back(std::move(parentChain)); decRefObj(this); } diff --git a/hphp/runtime/ext/asio/ext_concurrent-wait-handle.h b/hphp/runtime/ext/asio/ext_concurrent-wait-handle.h index a70c3e0108171b..1c8f6d99e023fd 100644 --- a/hphp/runtime/ext/asio/ext_concurrent-wait-handle.h +++ b/hphp/runtime/ext/asio/ext_concurrent-wait-handle.h @@ -17,6 +17,8 @@ #pragma once +#include + #include "hphp/runtime/base/type-array.h" #include "hphp/runtime/base/type-object.h" #include "hphp/runtime/ext/asio/ext_waitable-wait-handle.h" @@ -37,9 +39,28 @@ struct c_ConcurrentWaitHandle final : using SystemLib::ClassLoader<"HH\\ConcurrentWaitHandle">::className; static void instanceDtor(ObjectData* obj, const Class*) { auto wh = wait_handle(obj); - auto const sz = wh->heapSize(); - wh->~c_ConcurrentWaitHandle(); - tl_heap->objFree(obj, sz); + + std::vector queue = {wh}; + for (std::size_t i = 0; i < queue.size(); i++) { + auto cur = queue[i]; + for (int32_t j = 0; j < cur->m_cap; j++) { + auto cur_child = cur->m_children[j].m_child; + assertx(isFailed() || cur_child->isFinished()); + + if (cur_child->getKind() == Kind::Concurrent) { + if (cur_child->decReleaseCheck()) { + queue.push_back(cur_child->asConcurrent()); + } + } else { + decRefObj(cur_child); + } + } + } + + for (auto& cur : queue) { + auto const sz = cur->heapSize(); + tl_heap->objFree(cur, sz); + } } explicit c_ConcurrentWaitHandle(unsigned cap = 0) @@ -86,8 +107,8 @@ struct c_ConcurrentWaitHandle final : return getChildIdx() == getWaitHandle()->m_unfinished; } - void onUnblocked() { - getWaitHandle()->onUnblocked(getChildIdx()); + void onUnblocked(std::vector& worklist) { + getWaitHandle()->onUnblocked(getChildIdx(), worklist); } AsioBlockable m_blockable; @@ -100,7 +121,7 @@ struct c_ConcurrentWaitHandle final : } String getName(); - void onUnblocked(uint32_t idx); + void onUnblocked(uint32_t idx, std::vector& worklist); c_WaitableWaitHandle* getChild(); template void forEachChild(T fn); @@ -113,8 +134,8 @@ struct c_ConcurrentWaitHandle final : private: static req::ptr Alloc(int32_t cnt); void initialize(ContextStateIndex ctxStateIdx); - void markAsFinished(void); - void markAsFailed(const Object& exception); + void markAsFinished(std::vector& worklist); + void markAsFailed(const Object& exception, std::vector& worklist); void setState(uint8_t state) { setKindState(Kind::Concurrent, state); } private: diff --git a/hphp/runtime/ext/asio/ext_condition-wait-handle.cpp b/hphp/runtime/ext/asio/ext_condition-wait-handle.cpp index fbc125c48c72e6..4ada943d95d524 100644 --- a/hphp/runtime/ext/asio/ext_condition-wait-handle.cpp +++ b/hphp/runtime/ext/asio/ext_condition-wait-handle.cpp @@ -126,7 +126,7 @@ void c_ConditionWaitHandle::initialize(c_WaitableWaitHandle* child) { } } -void c_ConditionWaitHandle::onUnblocked() { +void c_ConditionWaitHandle::onUnblocked(std::vector& worklist) { decRefObj(m_child); m_child = nullptr; @@ -142,7 +142,7 @@ void c_ConditionWaitHandle::onUnblocked() { make_tv(getNotNotifiedException().detach()), m_resultOrException ); - parentChain.unblock(); + worklist.emplace_back(std::move(parentChain)); decRefObj(this); } diff --git a/hphp/runtime/ext/asio/ext_condition-wait-handle.h b/hphp/runtime/ext/asio/ext_condition-wait-handle.h index bb49388396cbd8..fd180da215dbf9 100644 --- a/hphp/runtime/ext/asio/ext_condition-wait-handle.h +++ b/hphp/runtime/ext/asio/ext_condition-wait-handle.h @@ -18,6 +18,8 @@ #ifndef incl_HPHP_EXT_ASIO_CONDITION_WAIT_HANDLE_H_ #define incl_HPHP_EXT_ASIO_CONDITION_WAIT_HANDLE_H_ +#include + #include "hphp/runtime/base/vanilla-dict.h" #include "hphp/runtime/ext/asio/ext_waitable-wait-handle.h" #include "hphp/runtime/ext/extension.h" @@ -48,7 +50,7 @@ struct c_ConditionWaitHandle final : } String getName(); - void onUnblocked(); + void onUnblocked(std::vector& worklist); c_WaitableWaitHandle* getChild(); static const int8_t STATE_BLOCKED = 2; diff --git a/hphp/runtime/ext/asio/ext_priority-bridge-wait-handle.cpp b/hphp/runtime/ext/asio/ext_priority-bridge-wait-handle.cpp index 29cc0962b44c9f..017da518da238a 100644 --- a/hphp/runtime/ext/asio/ext_priority-bridge-wait-handle.cpp +++ b/hphp/runtime/ext/asio/ext_priority-bridge-wait-handle.cpp @@ -80,7 +80,7 @@ void c_PriorityBridgeWaitHandle::initialize(c_WaitableWaitHandle* child) { } } -void c_PriorityBridgeWaitHandle::onUnblocked() { +void c_PriorityBridgeWaitHandle::onUnblocked(std::vector& worklist) { auto parentChain = getParentChain(); // Propagate the child's result. @@ -90,7 +90,7 @@ void c_PriorityBridgeWaitHandle::onUnblocked() { decRefObj(m_child); m_child = nullptr; - parentChain.unblock(); + worklist.emplace_back(std::move(parentChain)); decRefObj(this); } diff --git a/hphp/runtime/ext/asio/ext_priority-bridge-wait-handle.h b/hphp/runtime/ext/asio/ext_priority-bridge-wait-handle.h index f7dbe11086ce97..951f7b7c354041 100644 --- a/hphp/runtime/ext/asio/ext_priority-bridge-wait-handle.h +++ b/hphp/runtime/ext/asio/ext_priority-bridge-wait-handle.h @@ -17,6 +17,8 @@ #pragma once +#include + #include "hphp/runtime/ext/asio/ext_waitable-wait-handle.h" #include "hphp/runtime/ext/asio/ext_resumable-wait-handle.h" @@ -49,7 +51,7 @@ struct c_PriorityBridgeWaitHandle final } String getName(); - void onUnblocked(); + void onUnblocked(std::vector& worklist); c_WaitableWaitHandle* getChild(); // Prioritize the child of this PBWH. Will lift the child into the object's diff --git a/hphp/test/slow/async/long_awaitall_chain.php b/hphp/test/slow/async/long_awaitall_chain.php new file mode 100644 index 00000000000000..35b5b5ee1f5d98 --- /dev/null +++ b/hphp/test/slow/async/long_awaitall_chain.php @@ -0,0 +1,29 @@ + { + $s = new \HH\Lib\Async\Semaphore(10, async (bool $should_wait) ==> { + if ($should_wait) { + await Asio\usleep(20000*1000); + } else { + await Asio\later(); + } + }); + + concurrent { + await $s->waitForAsync(true); + await async { + for ($i = 0; $i < 1000000; $i++) { + await $s->waitForAsync(false); + } + + echo "$i\n"; + }; + } +} + +<<__EntryPoint>> +function main(): void { + Asio\join(bar()); +} diff --git a/hphp/test/slow/async/long_awaitall_chain.php.expect b/hphp/test/slow/async/long_awaitall_chain.php.expect new file mode 100644 index 00000000000000..749fce669df1b5 --- /dev/null +++ b/hphp/test/slow/async/long_awaitall_chain.php.expect @@ -0,0 +1 @@ +1000000