Skip to content

event loop: tolerate backward monotonic clock jumps - #369

Open
VijayabaskarR-06 wants to merge 2 commits into
bitcoin-core:masterfrom
VijayabaskarR-06:fix-nonmonotonic-clock
Open

VijayabaskarR-06 wants to merge 2 commits into
bitcoin-core:masterfrom
VijayabaskarR-06:fix-nonmonotonic-clock

Conversation

@VijayabaskarR-06

@VijayabaskarR-06 VijayabaskarR-06 commented Sep 29, 2026 •

Copy link
Copy Markdown

Description

Cap'n Proto versions before 1.2 assume that successive CLOCK_MONOTONIC values never decrease. In some Linux virtualized environments, however, the clock can briefly move backwards, causing the event-loop wait to fail with a KJ exception ending with:

"can't advance backwards in time"

Cap'n Proto addressed this upstream in capnproto/capnproto#2261 and capnproto/capnproto#2296. Newer versions clamp a regressed timestamp to the previous monotonic value instead of raising this error.

The failing check in older versions is recoverable (KJ_REQUIRE(newTime >= time, "can't advance backwards in time") { return; } in TimerImpl::advanceTo()). This PR installs a kj::ExceptionCallback on the event loop thread for the duration of EventLoop::loop(). The callback logs a warning and returns for this specific error, so the timer keeps its previous time like newer versions do. Every other error is forwarded to the next callback unchanged. The workaround is compiled only for Cap'n Proto versions before 1.2, so builds against newer versions are unchanged.

Testing

Adds a Linux-only mpclocktest that overrides clock_gettime() to force a 10 ms CLOCK_MONOTONIC regression while the event loop is waiting. It is also added to the sanitize CI job.

Tested locally in Linux containers:

  • Cap'n Proto 0.9.2, 1.0.1, 1.1.0 and 1.3.0: all tests pass, and mpclocktest passed 100+ repeated runs on each.
  • With 0.9.2 and the callback removed, the test fails with can't advance backwards in time.
  • 0.9.2 with the olddeps warning flags (-Werror), and clang with the llvm job's flags plus clang-tidy and IWYU on the changed files: clean.
  • TSan build (uninstrumented Cap'n Proto 1.1.0): mpclocktest 50/50 runs with no reports.

Related

@DrahtBot

DrahtBot commented Sep 29, 2026 •

Copy link
Copy Markdown

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline and AI policy for information on the review process.

Type Reviewers
Concept ACK ryanofsky

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Comment thread src/mp/proxy.cpp Outdated
KJ_IF_MAYBE(exception, kj::runCatchingExceptions([&] {
read_bytes = wait_stream->read(&buffer, 0, 1).wait(m_io_context.waitScope);
})) {
// Cap'n Proto versions before 1.2 throw if CLOCK_MONOTONIC briefly

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we gate this behind a version check (in addition to the string comparison below)?

@VijayabaskarR-06 VijayabaskarR-06 Sep 29, 2026 •

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So Basically I added them so the workaround is now actually gated with CAPNP_VERSION < 1002000 in addition to the exception type and message check so thats why Cap’n Proto 1.2+ won’t take this path.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that also makes it easier to drop the workaround if we ever bump the minimum version past this.

Comment thread src/mp/proxy.cpp Outdated
// Cap'n Proto versions before 1.2 throw if CLOCK_MONOTONIC briefly
// moves backwards. Retrying lets the event loop recover once the
// clock catches up.
if (exception->getType() != kj::Exception::Type::FAILED ||

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style nit: if (!( exception->getType() == kj::Exception::Type::FAILED && exception->getDescription ...)) is more readable imo.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it Changed to the positive condition form you suggested. Thanks.

Comment thread src/mp/proxy.cpp Outdated

kj::Own<kj::AsyncIoStream>& wait_stream{m_wait_stream};
char buffer = 0;
bool clock_regressed{false};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you need both clock_regressed and retry?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so i just removed retry now here!. clock_regressed is only kept to suppress duplicate warnings during consecutive clock regression failures and it gets reset after a successful wait.

@Sjors

Sjors commented Sep 29, 2026 •

Copy link
Copy Markdown
Member

Are you sure this is the only place in the codebase where can't advance backwards in time or a similar error can happen? In that case the workaround seems simple enough.

Consider dropping (most of) "Changes" and "Testing" from the PR description.

@VijayabaskarR-06

Copy link
Copy Markdown
Author

Hello @Sjors Thankyou very much for you validations ! I completely checked the wait sites in the repository and as pe my knowledge this is the only active KJ wait using m_io_context.waitScope. The other active waits are condition variable waits, so they don't go through KJ's timer handling and can't produce this exception.
I've also trimmed the PR description according to the changes as suggested. Thankyou once again would love to hear furthur feedbacks from you!

@Sjors

Sjors commented Sep 29, 2026

Copy link
Copy Markdown
Member

Codex (Astra) suggests the following additional test, to actually simulate the time shift:

patch
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index fd0aa023d..275454734 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -40,3 +40,14 @@ if(BUILD_TESTING AND TARGET CapnProto::kj-test)
   add_dependencies(mptests mptest)
   add_test(NAME mptest COMMAND mptest)
+
+  # This test overrides clock_gettime() to force CLOCK_MONOTONIC backwards.
+  # Use a separate executable so other tests keep using the system clock.
+  # Linux Cap'n Proto versions before 1.2 throw on these backward readings.
+  if(CMAKE_SYSTEM_NAME STREQUAL "Linux")
+    add_executable(mpclocktest mp/test/clock_tests.cpp)
+    target_link_libraries(mpclocktest PRIVATE Libmultiprocess::multiprocess CapnProto::kj-test Threads::Threads ${CMAKE_DL_LIBS})
+    add_dependencies(mptests mpclocktest)
+    add_test(NAME mpclocktest COMMAND mpclocktest)
+    set_tests_properties(mpclocktest PROPERTIES TIMEOUT 30)
+  endif()
 endif()
diff --git a/test/mp/test/clock_tests.cpp b/test/mp/test/clock_tests.cpp
new file mode 100644
index 000000000..e79d3d83d
--- /dev/null
+++ b/test/mp/test/clock_tests.cpp
@@ -0,0 +1,93 @@
+// Copyright (c) The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <mp/proxy-io.h>
+#include <mp/proxy.h>
+
+#include <capnp/common.h>
+#include <kj/async-io.h>
+#include <kj/common.h>
+#include <kj/debug.h>
+#include <kj/function.h>
+#include <kj/memory.h>
+#include <kj/test.h>
+#include <kj/time.h>
+#include <kj/timer.h>
+#include <kj/units.h>
+
+#include <cstdlib>
+#include <ctime>
+#include <dlfcn.h>
+#include <future>
+#include <string>
+#include <sys/types.h>
+#include <thread>
+
+namespace {
+struct ClockRegression
+{
+    timespec time{};
+    unsigned reads{0};
+    std::promise<void> entering_wait;
+};
+
+// Only the event-loop thread opts into the fake clock. In particular, CTest's
+// watchdog and the test thread's synchronization keep using the real clock.
+thread_local ClockRegression* g_regression{nullptr};
+} // namespace
+
+// Interpose both KJ's clock reads and calls from this executable, without
+// requiring LD_PRELOAD or a modified Cap'n Proto build.
+extern "C" int clock_gettime(clockid_t clock, struct timespec* time) noexcept
+{
+    if (clock == CLOCK_MONOTONIC && g_regression) {
+        *time = g_regression->time;
+        if (++g_regression->reads == 1) g_regression->entering_wait.set_value();
+        return 0;
+    }
+    static const auto real_clock_gettime = reinterpret_cast<decltype(&clock_gettime)>(dlsym(RTLD_NEXT, "clock_gettime"));
+    if (!real_clock_gettime) std::abort();
+    return real_clock_gettime(clock, time);
+}
+
+KJ_TEST("EventLoop recovers from a backward monotonic clock reading")
+{
+    ClockRegression regression;
+    unsigned warnings{0};
+    std::promise<mp::EventLoopRef> started;
+    std::thread thread([&] {
+        mp::EventLoop loop("mpclocktest", [&](mp::LogMessage log) {
+            if (log.level == mp::Log::Warning && log.message.find("non-monotonic clock read") != std::string::npos) ++warnings;
+        });
+        started.set_value(mp::EventLoopRef{loop});
+        loop.loop();
+    });
+    auto loop = started.get_future().get();
+
+    loop->sync([&] {
+        // Regress relative to KJ's last observed time, so scheduling delays
+        // cannot cause the real clock to catch up before the fault is observed.
+        const auto time = loop->m_io_context.provider->getTimer().now() - kj::origin<kj::TimePoint>() - 10 * kj::MILLISECONDS;
+        regression.time.tv_sec = time / kj::SECONDS;
+        regression.time.tv_nsec = (time % kj::SECONDS) / kj::NANOSECONDS;
+        g_regression = &regression;
+    });
+
+    // KJ reads the clock before entering epoll_wait(), and again after waking.
+    // Wait for the first read before posting work: this ensures the socket read
+    // is pending, instead of completing synchronously and bypassing KJ's timer.
+    regression.entering_wait.get_future().wait();
+    loop->sync([&] { g_regression = nullptr; });
+
+    // Check another callback and clean shutdown after restoring the real clock.
+    bool completed{false};
+    loop->sync([&] { completed = true; });
+    loop.reset();
+    thread.join();
+
+    KJ_EXPECT(regression.reads >= 2);
+    KJ_EXPECT(completed);
+    // Newer KJ versions clamp internally; older ones need the retry handler.
+    KJ_EXPECT(warnings == (CAPNP_VERSION < 1002000 ? 1u : 0u));
+}

It's probably overkill though, and I don't fully understand what it's doing.

@VijayabaskarR-06

Copy link
Copy Markdown
Author

Thanks @Sjors. So currently I just validated the suggested regression test and added it in here 261d155.
The test installs a regressed monotonic clock for the event loop thread and it waits until the event loop performs its first fake clock read, and only then posts work to wake it so the backward time path is actually exercised.So this algorithm can be more accurate as you said !

I also tested it with Cap’n Proto 0.9.2, 1.0.1, and 1.2.0, and all passed then i ran the regression test 100 consecutive times successfully. So waiting for your review!
Thanks for the suggestion this gives the workaround direct regression coverage.

@ryanofsky ryanofsky left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Note: review below is entirely from LLM, but I think the approach it suggests seems a bit cleaner and less fragile so would recommend trying it]

Concept ACK 261d155. Thanks for following up on bitcoin/bitcoin#36345. I suggested there that upgrading Cap'n Proto in CI might be enough, but as maflcko pointed out, the latest Ubuntu LTS only ships 1.1, so a workaround here makes sense for anyone building against distro packages.

I left a suggestion below for a less invasive way to do this with kj::ExceptionCallback, plus a few smaller comments. It would also be good to move the explanation in the PR description into the first commit message, since both commits currently have empty bodies.

Comment thread src/mp/proxy.cpp Outdated
#include <atomic>
#include <capnp/capability.h>
#include <capnp/common.h> // IWYU pragma: keep
#include <capnp/generated-header-support.h>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "event loop: tolerate backward monotonic clock jumps" (6254800)

This include is added here and removed again in the test commit, so the change could be dropped from both commits.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, dropped the include from both commits.

Comment thread src/mp/proxy.cpp Outdated
Comment on lines +304 to +323
for (;;) {
const size_t read_bytes = wait_stream->read(&buffer, 0, 1).wait(m_io_context.waitScope);
size_t read_bytes{0};
// Unlike try/catch, this works across KJ dynamic library boundaries.
KJ_IF_MAYBE(exception, kj::runCatchingExceptions([&] {
read_bytes = wait_stream->read(&buffer, 0, 1).wait(m_io_context.waitScope);
})) {
// Cap'n Proto versions before 1.2 throw if CLOCK_MONOTONIC briefly
// moves backwards. Retrying lets the event loop recover once the
// clock catches up.
if (!(CAPNP_VERSION < 1002000 &&
exception->getType() == kj::Exception::Type::FAILED &&
exception->getDescription().endsWith("can't advance backwards in time"))) {
kj::throwRecoverableException(kj::mv(*exception));
}
if (!clock_regressed) {
MP_LOG(*this, Log::Warning) << "EventLoop: retrying after non-monotonic clock read.";
clock_regressed = true;
}
continue;
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "event loop: tolerate backward monotonic clock jumps" (6254800)

Instead of catching the exception and retrying, I think a simpler and less invasive fix would be to install a kj::ExceptionCallback on the event loop thread that ignores this specific error, and leave the wait() call as it is on master.

This works because the check that fails in old Cap'n Proto versions is a recoverable one: KJ_REQUIRE(newTime >= time, "can't advance backwards in time") { return; } in TimerImpl::advanceTo(). KJ reports it by calling onRecoverableException() on the current thread's ExceptionCallback, and the default callback throws. If the callback returns instead, the { return; } recovery block runs and advanceTo() keeps the previous time, which is basically what the upstream fix in capnproto/capnproto#2296 does.

The callback is registered just by constructing it. The kj::ExceptionCallback constructor pushes the object onto a thread-local stack of callbacks (saving the previous one as next), and the destructor pops it. So a local variable at the top of EventLoop::loop() applies to everything KJ does on the event loop thread while the loop runs, and to nothing on other threads:

namespace {
#if CAPNP_VERSION < 1002000
//! Ignore "can't advance backwards in time" errors from Cap'n Proto versions
//! before 1.2, which raise them when CLOCK_MONOTONIC briefly goes backwards
//! (https://github.com/capnproto/capnproto/issues/2261). Returning lets
//! TimerImpl::advanceTo() keep the previous time, like newer versions do.
class ClockErrorCallback : public kj::ExceptionCallback
{
public:
    explicit ClockErrorCallback(EventLoop& loop) : m_loop(loop) {}
    void onRecoverableException(kj::Exception&& exception) override
    {
        if (exception.getType() == kj::Exception::Type::FAILED &&
            exception.getDescription().endsWith("can't advance backwards in time")) {
            MP_LOG(m_loop, Log::Warning) << "EventLoop: ignoring non-monotonic clock read.";
            return;
        }
        next.onRecoverableException(kj::mv(exception));
    }
    EventLoop& m_loop;
};
#endif
} // namespace

void EventLoop::loop()
{
    assert(!CurrentThread().loop_thread);
    CurrentThread().loop_thread = true;
    KJ_DEFER(CurrentThread().loop_thread = false);
#if CAPNP_VERSION < 1002000
    // Registered with KJ for this thread until loop() returns.
    ClockErrorCallback clock_error_callback{*this};
#endif
    ...rest of loop() unchanged from master...
}

Advantages over the current approach:

  • The exception is currently thrown from UnixEventPort::processEpollEvents() after epoll events have been consumed, so unwinding drops its return value, which says whether another thread woke the loop to run kj::Executor events. Nothing here uses kj::Executor so this doesn't break anything now, but it would be easy to miss later. With a callback, nothing is unwound and KJ's state stays the same as if the clock had not moved backwards.
  • It covers every KJ wait and poll on the event loop thread, not only this call site, which answers Sjors's question about other places this could happen.
  • Builds against Cap'n Proto 1.2 and later compile exactly the same code as master.

The new test should keep working as is, since it only counts warnings. I haven't tried this against an old Cap'n Proto version, so it's possible I'm missing something.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, switched to the kj::ExceptionCallback approach in f5ce6f8, close to your snippet.

Tested in Linux containers against Cap'n Proto 0.9.2, 1.0.1, 1.1.0 and 1.3.0. With 0.9.2, removing the callback makes the test fail with can't advance backwards in time, and with it the loop recovers.

One thing did need changing: the callback can run more than once during a single wakeup (I saw 1 or 2 warnings depending on timing), so the test now checks warnings >= 1 on older versions instead of exactly 1.

Comment thread src/mp/proxy.cpp Outdated
// Cap'n Proto versions before 1.2 throw if CLOCK_MONOTONIC briefly
// moves backwards. Retrying lets the event loop recover once the
// clock catches up.
if (!(CAPNP_VERSION < 1002000 &&

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "event loop: tolerate backward monotonic clock jumps" (6254800)

If you keep this approach rather than the ExceptionCallback one suggested above: since CAPNP_VERSION is a compile-time constant, it would be better to check it with #if CAPNP_VERSION < 1002000 around the whole runCatchingExceptions block. Right now, with Cap'n Proto 1.2 and later, every exception thrown from wait() is still caught and rethrown through kj::throwRecoverableException, which changes non-KJ exceptions into kj::Exception for no reason. With #if, newer builds compile the same code as before, and it's clear what to delete when the minimum version is raised.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved by the switch to the callback approach. The class and its registration are both under #if CAPNP_VERSION < 1002000, so builds against 1.2+ compile the same code as master.

Comment thread test/mp/test/clock_tests.cpp Outdated
Comment on lines +92 to +93
// Newer KJ versions clamp internally; older ones need the retry handler.
KJ_EXPECT(warnings == (CAPNP_VERSION < 1002000 ? 1u : 0u));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "test: cover backward monotonic clock recovery" (261d155)

I think it's worth keeping this test, since otherwise the workaround has no coverage. Note that the only Linux CI job using Cap'n Proto older than 1.2 is olddeps (0.9.2), so that's the job that actually runs the workaround path. The other jobs only check that newer versions clamp the time themselves. It would also be good to confirm the sanitize job (TSan, with Cap'n Proto built with TSan) is happy with the clock_gettime override. I approved the CI runs so we should see results soon.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kept the test. The previous CI run had three failures, now fixed in 02767e1:

  • sanitize builds only mptest but runs every registered test, so mpclocktest was "Not Run". Added it to that job's BUILD_TARGETS, so the clock_gettime override is now also checked under TSan. Locally, a TSan build passed 50/50 runs with no reports, but that used an uninstrumented Cap'n Proto, so the CI job is the real check.
  • default and llvm failed on IWYU wanting <kj/common.h> and <kj/debug.h> in clock_tests.cpp, now added.

Cap'n Proto versions before 1.2 assume that successive CLOCK_MONOTONIC
values never decrease. In some Linux virtualized environments the clock
can briefly move backwards, and the event loop wait then fails with a
KJ exception ending in "can't advance backwards in time". Cap'n Proto
fixed this upstream in capnproto/capnproto#2261 and
capnproto/capnproto#2296, where a regressed timestamp is clamped to the
previous value instead of raising an error.

The failing check in older versions is recoverable:
KJ_REQUIRE(newTime >= time, "can't advance backwards in time") { return; }
in TimerImpl::advanceTo(). KJ reports it by calling
onRecoverableException() on the current thread's kj::ExceptionCallback,
and the default callback throws. Install a callback on the event loop
thread that logs a warning and returns for this specific error, so the
recovery block runs and the timer keeps its previous time, which matches
the behavior of newer versions. All other errors are forwarded to the
next callback unchanged.

The callback only exists while EventLoop::loop() runs, applies only to
the event loop thread, and covers every KJ wait and poll on that thread.
It is compiled only for Cap'n Proto versions before 1.2, so builds
against newer versions are unchanged.
Add a Linux-only mpclocktest executable that overrides clock_gettime()
so CLOCK_MONOTONIC returns a time 10ms behind KJ's last observed time
while the event loop is waiting. It checks that the event loop keeps
running callbacks and shuts down cleanly after the real clock is
restored.

With Cap'n Proto versions before 1.2, the test expects at least one
"non-monotonic clock read" warning from the ClockErrorCallback added in
the previous commit. The callback can run more than once during a single
wakeup, so the exact count is not checked. With newer versions, KJ
clamps the time internally and no warning is expected.

The test is a separate executable so other tests keep using the system
clock. Add it to the sanitize CI job's build targets, since that job
only builds mptest but runs every registered test, and so the
clock_gettime() override is also checked under ThreadSanitizer.

@ryanofsky ryanofsky left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review 02767e1. Thanks for the updates! The ASan + UBSan Bitcoin Core CI job found a real problem with the ExceptionCallback approach I suggested: KJ checks that the callback is on the stack, and ASan's detect_stack_use_after_return option moves it off the stack. Details and a fix are in the comment below.

Apart from that, this looks good. I checked TimerImpl::advanceTo() at the v0.9.2, v1.0.1, v1.1.0 and v1.2.0 tags. The three older versions have the same recoverable KJ_REQUIRE(newTime >= time, "can't advance backwards in time") { return; } on Linux, and v1.2.0 replaces it with std::max, so the CAPNP_VERSION < 1002000 cutoff is right.

Comment thread src/mp/proxy.cpp
Comment on lines +291 to +294
//! Ignore "can't advance backwards in time" errors from Cap'n Proto versions
//! before 1.2, which raise them when CLOCK_MONOTONIC briefly goes backwards
//! (https://github.com/capnproto/capnproto/issues/2261). Returning lets
//! TimerImpl::advanceTo() keep the previous time, like newer versions do.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "event loop: tolerate backward monotonic clock jumps" (f5ce6f8)

Note for reviewers: The comment above says that returning from the callback lets TimerImpl::advanceTo() keep the previous time "like newer versions do". Here are the two versions of advanceTo() it compares, from Cap'n Proto's kj/timer.c++:

// Before 1.2 (non-Apple). When the callback returns, the { return; } block runs.
KJ_REQUIRE(newTime >= time, "can't advance backwards in time") { return; }
time = newTime;
for (;;) { /* fire timers with deadline <= time */ }

// 1.2 and later.
time = std::max(time, newTime);
for (;;) { /* fire timers with deadline <= time */ }

When the clock goes backwards, both leave time unchanged, so the comment is right about that. The difference is that 1.2 still runs the loop that fires timers due at or before time, while the old version returns before reaching it.

That doesn't matter in practice. Timers due at or before time normally already fired during the previous advanceTo() call, the one that set time. The only exception is a timer added after that call with a deadline at or before time, for example afterDelay(0). Version 1.2 fires it right away, and with this workaround it fires on the next advanceTo() call instead. That call comes soon: the event loop sets its wait timeout to the timer's deadline minus the current clock reading, and since the clock is behind by the size of the regression, the timeout is at most that size (microseconds in the reports in capnproto/capnproto#2261). Nothing to change here.

Comment thread src/mp/proxy.cpp
Comment on lines +318 to +321
#if CAPNP_VERSION < 1002000
// Registered with KJ for this thread until loop() returns.
ClockErrorCallback clock_error_callback{*this};
#endif

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "event loop: tolerate backward monotonic clock jumps" (f5ce6f8)

Fix is needed here. The ASan + UBSan Bitcoin Core CI job (log) shows a problem with this approach, which is my fault for suggesting it. test_bitcoin aborts on the first IPC test with:

kj/exception.c++:1039: failed: expected offset < 65536 && offset > -65536; ExceptionCallback must be allocated on the stack.

The kj::ExceptionCallback constructor checks that the object is within 64 KiB of a local variable in the constructor. KJ skips that check when KJ itself is built with ASan, but in this job only Bitcoin Core and libmultiprocess are built with ASan, linked against Ubuntu's uninstrumented Cap'n Proto 1.1. The job also sets ASAN_OPTIONS=detect_stack_use_after_return=1, which makes ASan move local variables like clock_error_callback into a heap-allocated "fake stack", so the check fails and the exception terminates the event loop thread. Bitcoin Core's own ASan CI job uses distro Cap'n Proto and the same option, so it would hit this too after a subtree update.

There's no way to turn the check off from outside KJ, but it works to disable ASan instrumentation for loop() and move the rest of its body into a separate method, so only the small function that registers the callback is uninstrumented. A diff on top of this PR:

diff --git a/include/mp/proxy-io.h b/include/mp/proxy-io.h
index b226284..9e3bd29 100644
--- a/include/mp/proxy-io.h
+++ b/include/mp/proxy-io.h
@@ -266,6 +266,10 @@ public:
     //! the m_num_refs reference count is 0.
     void loop();

+    //! Implementation of loop(), separate so loop() can be excluded from ASan
+    //! instrumentation.
+    void loopImpl();
+
     //! Run function on event loop thread. Does not return until function completes.
     //! Must be called while the loop() function is active.
     void sync(kj::FunctionParam<void()> fn);
diff --git a/src/mp/proxy.cpp b/src/mp/proxy.cpp
index 20ce5a7..cb4e475 100644
--- a/src/mp/proxy.cpp
+++ b/src/mp/proxy.cpp
@@ -310,6 +310,15 @@ public:
 #endif
 } // namespace

+// ASan is disabled for this function so clock_error_callback stays on the real
+// stack. KJ requires ExceptionCallback objects to be on the stack, but ASan's
+// detect_stack_use_after_return option moves local variables to a
+// heap-allocated fake stack, and KJ only skips its check when KJ itself is
+// built with ASan. The event loop code is in loopImpl(), which is still
+// instrumented.
+#if defined(__GNUC__) || defined(__clang__)
+__attribute__((no_sanitize("address")))
+#endif
 void EventLoop::loop()
 {
     assert(!CurrentThread().loop_thread);
@@ -319,7 +328,11 @@ void EventLoop::loop()
     // Registered with KJ for this thread until loop() returns.
     ClockErrorCallback clock_error_callback{*this};
 #endif
+    loopImpl();
+}

+void EventLoop::loopImpl()
+{
     {
         const Lock lock(m_mutex);
         assert(!m_async_fns);

I tested this by building with ASan against Cap'n Proto 1.2 with the version check forced on, so the callback is registered. With detect_stack_use_after_return=1, mptest and mpclocktest crash with the current code and pass with this change, with both clang and gcc. I also checked that a stack buffer overflow added to loopImpl() is still reported by ASan, so the compiler doesn't inline it into the uninstrumented loop().

Comment thread src/mp/proxy.cpp
}

namespace {
#if CAPNP_VERSION < 1002000

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "event loop: tolerate backward monotonic clock jumps" (f5ce6f8)

nit: I'd suggest dropping the #if CAPNP_VERSION < 1002000 around the ClockErrorCallback definition and keeping it only around the registration in loop(). That way the class still gets compiled with Cap'n Proto 1.2 and later, so if a refactoring breaks it, that shows up right away in every build instead of only later in olddeps or in builds against distro packages. The comment above the class could mention that it is only used with versions before 1.2, so it's still clear what to delete when the minimum version is raised.

Comment thread src/mp/proxy.cpp
//! before 1.2, which raise them when CLOCK_MONOTONIC briefly goes backwards
//! (https://github.com/capnproto/capnproto/issues/2261). Returning lets
//! TimerImpl::advanceTo() keep the previous time, like newer versions do.
class ClockErrorCallback : public kj::ExceptionCallback

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "event loop: tolerate backward monotonic clock jumps" (f5ce6f8)

Note: Not for this PR, but in case anyone wants to pick it up, I think the stack check that caused the ASan failure (comment above) could be fixed upstream in Cap'n Proto with a small patch, so kj::ExceptionCallback works in programs built with ASan against an uninstrumented libkj. That's the usual setup for sanitizer builds against distro packages, including the ASan CI jobs here and in Bitcoin Core.

The check is requireOnStack() in kj/exception.c++, which is also used by DisallowAsyncDestructorsScope and AllowAsyncDestructorsScope, and is unchanged on the v2 branch. It is skipped only when libkj itself is compiled with ASan or HWASan (capnproto/capnproto#1427, fixed in capnproto/capnproto#1478, and capnproto/capnproto#2500, where the maintainer said he was happy to accept a PR adding another condition). Since that is decided when libkj is built, programs built with ASan against a libkj built without it still fail the check whenever detect_stack_use_after_return moves the object to ASan's fake stack.

A fix that works regardless of how libkj was built would be to declare the public ASan functions from sanitizer/asan_interface.h as weak symbols and return early when the object is in the current thread's fake stack:

extern "C" {
void* __asan_get_current_fake_stack() __attribute__((weak));
void* __asan_addr_is_in_fake_stack(void* fake_stack, void* addr, void** beg, void** end) __attribute__((weak));
}

void requireOnStack(void* ptr, kj::StringPtr description) {
  if (__asan_get_current_fake_stack && __asan_addr_is_in_fake_stack) {
    void* fake_stack = __asan_get_current_fake_stack();
    if (fake_stack && __asan_addr_is_in_fake_stack(fake_stack, ptr, nullptr, nullptr)) return;
  }
  ...existing check...
}

I tried this in a small stand-in for libkj built without ASan, with a program built with and without ASan, and with detect_stack_use_after_return on and off. Objects on the stack passed in every case, heap-allocated objects were still rejected, and builds without ASan behaved the same as before. This also keeps the check working in ASan builds, while the current compile-time condition turns it off. Things still to work out for an upstream PR are Windows, which doesn't have weak symbols the same way, and HWASan, which has no fake stack, so capnproto/capnproto#2500 is a different case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants