Skip to content

Commit adac608

Browse files
authored
Defuse throttled_func when it's accidentally engaged (#18235)
This change prevents `throttled_func` from reading uninitialized memory under some yet-unkown circumstances. The tl;dr is: This simply moves the callback invocation into the storage. That way we can centrally avoid invoking the callback accidentally.
1 parent 09d8ac4 commit adac608

File tree

2 files changed

+26
-21
lines changed

2 files changed

+26
-21
lines changed

src/cascadia/WinRTUtils/inc/ThrottledFunc.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ class ThrottledFunc : public std::enable_shared_from_this<ThrottledFunc<leading,
113113
{
114114
try
115115
{
116-
std::apply(self->_func, self->_storage.take());
116+
self->_storage.apply(self->_func);
117117
}
118118
CATCH_LOG();
119119
}

src/inc/til/throttled_func.h

+25-20
Original file line numberDiff line numberDiff line change
@@ -30,12 +30,22 @@ namespace til
3030
}
3131
}
3232

33-
std::tuple<Args...> take()
33+
void apply(const auto& func)
3434
{
35-
std::unique_lock guard{ _lock };
36-
auto pendingRunArgs = std::move(*_pendingRunArgs);
37-
_pendingRunArgs.reset();
38-
return pendingRunArgs;
35+
decltype(_pendingRunArgs) args;
36+
{
37+
std::unique_lock guard{ _lock };
38+
args = std::exchange(_pendingRunArgs, std::nullopt);
39+
}
40+
// Theoretically it should always have a value, because the throttled_func
41+
// should not call the callback without there being a reason.
42+
// But in practice a failure here was observed at least once.
43+
// It's unknown to me what caused it, so the best we can do is avoid a crash.
44+
assert(args.has_value());
45+
if (args)
46+
{
47+
std::apply(func, *args);
48+
}
3949
}
4050

4151
explicit operator bool() const
@@ -60,10 +70,12 @@ namespace til
6070
return _isPending.exchange(true, std::memory_order_relaxed);
6171
}
6272

63-
std::tuple<> take()
73+
void apply(const auto& func)
6474
{
65-
reset();
66-
return {};
75+
if (_isPending.exchange(false, std::memory_order_relaxed))
76+
{
77+
func();
78+
}
6779
}
6880

6981
void reset()
@@ -171,31 +183,24 @@ namespace til
171183
void flush()
172184
{
173185
WaitForThreadpoolTimerCallbacks(_timer.get(), true);
174-
if (_storage)
175-
{
176-
_trailing_edge();
177-
}
186+
_timer_callback(nullptr, this, nullptr);
178187
}
179188

180189
private:
181190
static void __stdcall _timer_callback(PTP_CALLBACK_INSTANCE /*instance*/, PVOID context, PTP_TIMER /*timer*/) noexcept
182191
try
183192
{
184-
static_cast<throttled_func*>(context)->_trailing_edge();
185-
}
186-
CATCH_LOG()
187-
188-
void _trailing_edge()
189-
{
193+
const auto self = static_cast<throttled_func*>(context);
190194
if constexpr (Leading)
191195
{
192-
_storage.reset();
196+
self->_storage.reset();
193197
}
194198
else
195199
{
196-
std::apply(_func, _storage.take());
200+
self->_storage.apply(self->_func);
197201
}
198202
}
203+
CATCH_LOG()
199204

200205
wil::unique_threadpool_timer _createTimer()
201206
{

0 commit comments

Comments
 (0)