Skip to content

Commit 5e11458

Browse files
mszabo-wikiamszabo-wikia
committed
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<void> { $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.
1 parent 7ef6a12 commit 5e11458

11 files changed

+141
-59
lines changed

‎hphp/runtime/ext/asio/asio-blockable.cpp‎

Lines changed: 29 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -166,29 +166,35 @@ c_WaitableWaitHandle* AsioBlockable::getWaitHandle() const {
166166
}
167167

168168
void AsioBlockableChain::unblock() {
169-
while (auto cur = m_lastParent) {
170-
m_lastParent = cur->getPrevParent();
171-
cur->updatePrevParent(nullptr);
172-
// the onUnblocked handler may free cur
173-
switch (cur->getKind()) {
174-
case Kind::AsyncFunctionWaitHandleNode:
175-
getAsyncFunctionWaitHandleNode(cur)->onUnblocked();
176-
break;
177-
case Kind::AsyncGeneratorWaitHandle:
178-
getAsyncGeneratorWaitHandle(cur)->onUnblocked();
179-
break;
180-
case Kind::AwaitAllWaitHandleNode:
181-
getAwaitAllWaitHandleNode(cur)->onUnblocked();
182-
break;
183-
case Kind::ConcurrentWaitHandleNode:
184-
getConcurrentWaitHandleNode(cur)->onUnblocked();
185-
break;
186-
case Kind::ConditionWaitHandle:
187-
getConditionWaitHandle(cur)->onUnblocked();
188-
break;
189-
case Kind::PriorityBridgeWaitHandle:
190-
getPriorityBridgeWaitHandle(cur)->onUnblocked();
191-
break;
169+
std::vector<AsioBlockableChain> worklist = { *this };
170+
while (!worklist.empty()) {
171+
auto const lastParent = worklist.back().m_lastParent;
172+
worklist.pop_back();
173+
174+
for (AsioBlockable* cur = lastParent, *next; cur; cur = next) {
175+
next = cur->getPrevParent();
176+
cur->updatePrevParent(nullptr);
177+
// the onUnblocked handler may free cur
178+
switch (cur->getKind()) {
179+
case Kind::AsyncFunctionWaitHandleNode:
180+
getAsyncFunctionWaitHandleNode(cur)->onUnblocked();
181+
break;
182+
case Kind::AsyncGeneratorWaitHandle:
183+
getAsyncGeneratorWaitHandle(cur)->onUnblocked();
184+
break;
185+
case Kind::AwaitAllWaitHandleNode:
186+
getAwaitAllWaitHandleNode(cur)->onUnblocked(worklist);
187+
break;
188+
case Kind::ConcurrentWaitHandleNode:
189+
getConcurrentWaitHandleNode(cur)->onUnblocked(worklist);
190+
break;
191+
case Kind::ConditionWaitHandle:
192+
getConditionWaitHandle(cur)->onUnblocked(worklist);
193+
break;
194+
case Kind::PriorityBridgeWaitHandle:
195+
getPriorityBridgeWaitHandle(cur)->onUnblocked(worklist);
196+
break;
197+
}
192198
}
193199
}
194200
}

‎hphp/runtime/ext/asio/ext_await-all-wait-handle.cpp‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,7 @@ void c_AwaitAllWaitHandle::initialize(ContextStateIndex ctxStateIdx) {
149149
}
150150
}
151151

152-
void c_AwaitAllWaitHandle::onUnblocked(uint32_t idx) {
152+
void c_AwaitAllWaitHandle::onUnblocked(uint32_t idx, std::vector<AsioBlockableChain>& worklist) {
153153
assertx(idx <= m_unfinished);
154154
assertx(getState() == STATE_BLOCKED);
155155

@@ -166,25 +166,25 @@ void c_AwaitAllWaitHandle::onUnblocked(uint32_t idx) {
166166
} catch (const Object& cycle_exception) {
167167
assertx(cycle_exception->instanceof(SystemLib::getThrowableClass()));
168168
throwable_recompute_backtrace_from_wh(cycle_exception.get(), this);
169-
markAsFailed(cycle_exception);
169+
markAsFailed(cycle_exception, worklist);
170170
}
171171
return;
172172
}
173173
}
174174
// All children finished.
175-
markAsFinished();
175+
markAsFinished(worklist);
176176
}
177177
}
178178

179-
void c_AwaitAllWaitHandle::markAsFinished() {
179+
void c_AwaitAllWaitHandle::markAsFinished(std::vector<AsioBlockableChain>& worklist) {
180180
auto parentChain = getParentChain();
181181
setState(STATE_SUCCEEDED);
182182
tvWriteNull(m_resultOrException);
183-
parentChain.unblock();
183+
worklist.emplace_back(std::move(parentChain));
184184
decRefObj(this);
185185
}
186186

187-
void c_AwaitAllWaitHandle::markAsFailed(const Object& exception) {
187+
void c_AwaitAllWaitHandle::markAsFailed(const Object& exception, std::vector<AsioBlockableChain>& worklist) {
188188
for (uint32_t idx = 0; idx < m_cap; idx++) {
189189
auto const child = m_children[idx].m_child;
190190
if (!child->isFinished()) {
@@ -196,7 +196,7 @@ void c_AwaitAllWaitHandle::markAsFailed(const Object& exception) {
196196
auto parentChain = getParentChain();
197197
setState(STATE_FAILED);
198198
tvWriteObject(exception.get(), &m_resultOrException);
199-
parentChain.unblock();
199+
worklist.emplace_back(std::move(parentChain));
200200
decRefObj(this);
201201
}
202202

‎hphp/runtime/ext/asio/ext_await-all-wait-handle.h‎

Lines changed: 29 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@
1717

1818
#pragma once
1919

20+
#include <vector>
21+
2022
#include "hphp/runtime/base/type-array.h"
2123
#include "hphp/runtime/base/type-object.h"
2224
#include "hphp/runtime/ext/asio/ext_waitable-wait-handle.h"
@@ -37,9 +39,28 @@ struct c_AwaitAllWaitHandle final : c_WaitableWaitHandle,
3739

3840
static void instanceDtor(ObjectData* obj, const Class*) {
3941
auto wh = wait_handle<c_AwaitAllWaitHandle>(obj);
40-
auto const sz = wh->heapSize();
41-
wh->~c_AwaitAllWaitHandle();
42-
tl_heap->objFree(obj, sz);
42+
43+
std::vector<c_AwaitAllWaitHandle*> queue = {wh};
44+
for (std::size_t i = 0; i < queue.size(); i++) {
45+
auto cur = queue[i];
46+
for (int32_t j = 0; j < cur->m_cap; j++) {
47+
auto cur_child = cur->m_children[j].m_child;
48+
assertx(isFailed() || cur_child->isFinished());
49+
50+
if (cur_child->getKind() == Kind::AwaitAll) {
51+
if (cur_child->decReleaseCheck()) {
52+
queue.push_back(cur_child->asAwaitAll());
53+
}
54+
} else {
55+
decRefObj(cur_child);
56+
}
57+
}
58+
}
59+
60+
for (auto& cur : queue) {
61+
auto const sz = cur->heapSize();
62+
tl_heap->objFree(cur, sz);
63+
}
4364
}
4465

4566
explicit c_AwaitAllWaitHandle(unsigned cap = 0)
@@ -79,8 +100,8 @@ struct c_AwaitAllWaitHandle final : c_WaitableWaitHandle,
79100
return getChildIdx() == getWaitHandle()->m_unfinished;
80101
}
81102

82-
void onUnblocked() {
83-
getWaitHandle()->onUnblocked(getChildIdx());
103+
void onUnblocked(std::vector<AsioBlockableChain>& worklist) {
104+
getWaitHandle()->onUnblocked(getChildIdx(), worklist);
84105
}
85106

86107
AsioBlockable m_blockable;
@@ -93,7 +114,7 @@ struct c_AwaitAllWaitHandle final : c_WaitableWaitHandle,
93114
}
94115

95116
String getName();
96-
void onUnblocked(uint32_t idx);
117+
void onUnblocked(uint32_t idx, std::vector<AsioBlockableChain>& worklist);
97118
c_WaitableWaitHandle* getChild();
98119
template<typename T> void forEachChild(T fn);
99120

@@ -108,8 +129,8 @@ struct c_AwaitAllWaitHandle final : c_WaitableWaitHandle,
108129
static Object Create(Iter iter);
109130
static req::ptr<c_AwaitAllWaitHandle> Alloc(int32_t cnt);
110131
void initialize(ContextStateIndex ctxStateIdx);
111-
void markAsFinished(void);
112-
void markAsFailed(const Object& exception);
132+
void markAsFinished(std::vector<AsioBlockableChain>& worklist);
133+
void markAsFailed(const Object& exception, std::vector<AsioBlockableChain>& worklist);
113134
void setState(uint8_t state) { setKindState(Kind::AwaitAll, state); }
114135

115136
// Construct an AAWH from an array-like without making layout assumptions.

‎hphp/runtime/ext/asio/ext_concurrent-wait-handle.cpp‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ void c_ConcurrentWaitHandle::initialize(ContextStateIndex ctxStateIdx) {
101101
}
102102
}
103103

104-
void c_ConcurrentWaitHandle::onUnblocked(uint32_t idx) {
104+
void c_ConcurrentWaitHandle::onUnblocked(uint32_t idx, std::vector<AsioBlockableChain>& worklist) {
105105
assertx(idx <= m_unfinished);
106106
assertx(getState() == STATE_BLOCKED);
107107

@@ -118,25 +118,25 @@ void c_ConcurrentWaitHandle::onUnblocked(uint32_t idx) {
118118
} catch (const Object& cycle_exception) {
119119
assertx(cycle_exception->instanceof(SystemLib::getThrowableClass()));
120120
throwable_recompute_backtrace_from_wh(cycle_exception.get(), this);
121-
markAsFailed(cycle_exception);
121+
markAsFailed(cycle_exception, worklist);
122122
}
123123
return;
124124
}
125125
}
126126
// All children finished.
127-
markAsFinished();
127+
markAsFinished(worklist);
128128
}
129129
}
130130

131-
void c_ConcurrentWaitHandle::markAsFinished() {
131+
void c_ConcurrentWaitHandle::markAsFinished(std::vector<AsioBlockableChain>& worklist) {
132132
auto parentChain = getParentChain();
133133
setState(STATE_SUCCEEDED);
134134
tvWriteNull(m_resultOrException);
135-
parentChain.unblock();
135+
worklist.emplace_back(std::move(parentChain));
136136
decRefObj(this);
137137
}
138138

139-
void c_ConcurrentWaitHandle::markAsFailed(const Object& exception) {
139+
void c_ConcurrentWaitHandle::markAsFailed(const Object& exception, std::vector<AsioBlockableChain>& worklist) {
140140
for (uint32_t idx = 0; idx < m_cap; idx++) {
141141
auto const child = m_children[idx].m_child;
142142
if (!child->isFinished()) {
@@ -148,7 +148,7 @@ void c_ConcurrentWaitHandle::markAsFailed(const Object& exception) {
148148
auto parentChain = getParentChain();
149149
setState(STATE_FAILED);
150150
tvWriteObject(exception.get(), &m_resultOrException);
151-
parentChain.unblock();
151+
worklist.emplace_back(std::move(parentChain));
152152
decRefObj(this);
153153
}
154154

‎hphp/runtime/ext/asio/ext_concurrent-wait-handle.h‎

Lines changed: 29 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@
1717

1818
#pragma once
1919

20+
#include <vector>
21+
2022
#include "hphp/runtime/base/type-array.h"
2123
#include "hphp/runtime/base/type-object.h"
2224
#include "hphp/runtime/ext/asio/ext_waitable-wait-handle.h"
@@ -37,9 +39,28 @@ struct c_ConcurrentWaitHandle final :
3739
using SystemLib::ClassLoader<"HH\\ConcurrentWaitHandle">::className;
3840
static void instanceDtor(ObjectData* obj, const Class*) {
3941
auto wh = wait_handle<c_ConcurrentWaitHandle>(obj);
40-
auto const sz = wh->heapSize();
41-
wh->~c_ConcurrentWaitHandle();
42-
tl_heap->objFree(obj, sz);
42+
43+
std::vector<c_ConcurrentWaitHandle*> queue = {wh};
44+
for (std::size_t i = 0; i < queue.size(); i++) {
45+
auto cur = queue[i];
46+
for (int32_t j = 0; j < cur->m_cap; j++) {
47+
auto cur_child = cur->m_children[j].m_child;
48+
assertx(isFailed() || cur_child->isFinished());
49+
50+
if (cur_child->getKind() == Kind::Concurrent) {
51+
if (cur_child->decReleaseCheck()) {
52+
queue.push_back(cur_child->asConcurrent());
53+
}
54+
} else {
55+
decRefObj(cur_child);
56+
}
57+
}
58+
}
59+
60+
for (auto& cur : queue) {
61+
auto const sz = cur->heapSize();
62+
tl_heap->objFree(cur, sz);
63+
}
4364
}
4465

4566
explicit c_ConcurrentWaitHandle(unsigned cap = 0)
@@ -86,8 +107,8 @@ struct c_ConcurrentWaitHandle final :
86107
return getChildIdx() == getWaitHandle()->m_unfinished;
87108
}
88109

89-
void onUnblocked() {
90-
getWaitHandle()->onUnblocked(getChildIdx());
110+
void onUnblocked(std::vector<AsioBlockableChain>& worklist) {
111+
getWaitHandle()->onUnblocked(getChildIdx(), worklist);
91112
}
92113

93114
AsioBlockable m_blockable;
@@ -100,7 +121,7 @@ struct c_ConcurrentWaitHandle final :
100121
}
101122

102123
String getName();
103-
void onUnblocked(uint32_t idx);
124+
void onUnblocked(uint32_t idx, std::vector<AsioBlockableChain>& worklist);
104125
c_WaitableWaitHandle* getChild();
105126
template<typename T> void forEachChild(T fn);
106127

@@ -113,8 +134,8 @@ struct c_ConcurrentWaitHandle final :
113134
private:
114135
static req::ptr<c_ConcurrentWaitHandle> Alloc(int32_t cnt);
115136
void initialize(ContextStateIndex ctxStateIdx);
116-
void markAsFinished(void);
117-
void markAsFailed(const Object& exception);
137+
void markAsFinished(std::vector<AsioBlockableChain>& worklist);
138+
void markAsFailed(const Object& exception, std::vector<AsioBlockableChain>& worklist);
118139
void setState(uint8_t state) { setKindState(Kind::Concurrent, state); }
119140

120141
private:

‎hphp/runtime/ext/asio/ext_condition-wait-handle.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ void c_ConditionWaitHandle::initialize(c_WaitableWaitHandle* child) {
126126
}
127127
}
128128

129-
void c_ConditionWaitHandle::onUnblocked() {
129+
void c_ConditionWaitHandle::onUnblocked(std::vector<AsioBlockableChain>& worklist) {
130130
decRefObj(m_child);
131131
m_child = nullptr;
132132

@@ -142,7 +142,7 @@ void c_ConditionWaitHandle::onUnblocked() {
142142
make_tv<KindOfObject>(getNotNotifiedException().detach()),
143143
m_resultOrException
144144
);
145-
parentChain.unblock();
145+
worklist.emplace_back(std::move(parentChain));
146146
decRefObj(this);
147147
}
148148

‎hphp/runtime/ext/asio/ext_condition-wait-handle.h‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@
1818
#ifndef incl_HPHP_EXT_ASIO_CONDITION_WAIT_HANDLE_H_
1919
#define incl_HPHP_EXT_ASIO_CONDITION_WAIT_HANDLE_H_
2020

21+
#include <vector>
22+
2123
#include "hphp/runtime/base/vanilla-dict.h"
2224
#include "hphp/runtime/ext/asio/ext_waitable-wait-handle.h"
2325
#include "hphp/runtime/ext/extension.h"
@@ -48,7 +50,7 @@ struct c_ConditionWaitHandle final :
4850
}
4951

5052
String getName();
51-
void onUnblocked();
53+
void onUnblocked(std::vector<AsioBlockableChain>& worklist);
5254
c_WaitableWaitHandle* getChild();
5355

5456
static const int8_t STATE_BLOCKED = 2;

‎hphp/runtime/ext/asio/ext_priority-bridge-wait-handle.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ void c_PriorityBridgeWaitHandle::initialize(c_WaitableWaitHandle* child) {
8080
}
8181
}
8282

83-
void c_PriorityBridgeWaitHandle::onUnblocked() {
83+
void c_PriorityBridgeWaitHandle::onUnblocked(std::vector<AsioBlockableChain>& worklist) {
8484
auto parentChain = getParentChain();
8585

8686
// Propagate the child's result.
@@ -90,7 +90,7 @@ void c_PriorityBridgeWaitHandle::onUnblocked() {
9090
decRefObj(m_child);
9191
m_child = nullptr;
9292

93-
parentChain.unblock();
93+
worklist.emplace_back(std::move(parentChain));
9494
decRefObj(this);
9595
}
9696

‎hphp/runtime/ext/asio/ext_priority-bridge-wait-handle.h‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@
1717

1818
#pragma once
1919

20+
#include <vector>
21+
2022
#include "hphp/runtime/ext/asio/ext_waitable-wait-handle.h"
2123
#include "hphp/runtime/ext/asio/ext_resumable-wait-handle.h"
2224

@@ -49,7 +51,7 @@ struct c_PriorityBridgeWaitHandle final
4951
}
5052

5153
String getName();
52-
void onUnblocked();
54+
void onUnblocked(std::vector<AsioBlockableChain>& worklist);
5355
c_WaitableWaitHandle* getChild();
5456

5557
// Prioritize the child of this PBWH. Will lift the child into the object's

0 commit comments

Comments
 (0)