From 767ace45a739a69e67e7c8597515c32b7e261858 Mon Sep 17 00:00:00 2001 From: Dmytro Bezverkhnii Date: Wed, 16 Sep 2026 10:20:20 +0300 Subject: [PATCH 1/2] Fix: wait out a running IDLE before freeing its stream COR-221 Cherry-pick of upstream 03a19472 "Fix IMAP IDLE teardown races", adapted to this fork. setupIdle(), interruptIdle() and unsetupIdle() dereferenced mImap->imap_stream guarded only by mIdleEnabled, and idle() read the stream with no lock at all before blocking in mailstream_wait_idle() on it - so a teardown running meanwhile closed and freed the stream under the thread still using it. mIdleInProgress plus a condition variable close both: idle() takes a local mailimap* under the lock and raises the flag, teardown interrupts the idle and waits for the flag to drop before freeing anything, and the three main-thread entry points get their NULL checks. The fork locks through the MCB_* shim rather than raw pthread calls, so upstream's pthread_cond_* usage arrives as new MCB_COND_* macros in MCBasicLock.h, mapping to CONDITION_VARIABLE on Windows. Co-Authored-By: Claude Opus 5 --- src/async/imap/MCIMAPIdleOperation.cpp | 36 +++++++++-- src/async/imap/MCIMAPIdleOperation.h | 3 + src/core/basetypes/MCBasicLock.h | 13 ++++ src/core/imap/MCIMAPSession.cpp | 70 ++++++++++++++++------ src/core/imap/MCIMAPSession.h | 4 ++ src/include/MailCore/MCBasicLock.h | 13 ++++ src/include/MailCore/MCIMAPIdleOperation.h | 3 + src/include/MailCore/MCIMAPSession.h | 4 ++ unittest/IMAPIdleCancellationTests.swift | 60 ++++++++++++------- 9 files changed, 161 insertions(+), 45 deletions(-) diff --git a/src/async/imap/MCIMAPIdleOperation.cpp b/src/async/imap/MCIMAPIdleOperation.cpp index f911c1ce8..48eb879a1 100644 --- a/src/async/imap/MCIMAPIdleOperation.cpp +++ b/src/async/imap/MCIMAPIdleOperation.cpp @@ -39,16 +39,16 @@ uint32_t IMAPIdleOperation::lastKnownUID() void IMAPIdleOperation::prepare(void * data) { if (isInterrupted()) { - mSetupSuccess = false; + setSetupSuccess(false); return; } - mSetupSuccess = session()->session()->setupIdle(); + setSetupSuccess(session()->session()->setupIdle()); } void IMAPIdleOperation::unprepare(void * data) { - if (mSetupSuccess) { + if (setupSuccess()) { session()->session()->unsetupIdle(); } } @@ -61,6 +61,22 @@ bool IMAPIdleOperation::isInterrupted() { return interrupted; } +void IMAPIdleOperation::setSetupSuccess(bool setupSuccess) +{ + MCB_LOCK(&mLock); + mSetupSuccess = setupSuccess; + MCB_UNLOCK(&mLock); +} + +bool IMAPIdleOperation::setupSuccess() +{ + MCB_LOCK(&mLock); + bool setupSuccess = mSetupSuccess; + MCB_UNLOCK(&mLock); + + return setupSuccess; +} + void IMAPIdleOperation::main() { if (isInterrupted()) { @@ -76,7 +92,7 @@ void IMAPIdleOperation::main() performMethodOnCallbackThread((Object::Method) &IMAPIdleOperation::prepare, NULL, true); - if (!mSetupSuccess) { + if (!setupSuccess()) { return; } @@ -86,13 +102,21 @@ void IMAPIdleOperation::main() performMethodOnCallbackThread((Object::Method) &IMAPIdleOperation::unprepare, NULL, true); } +void IMAPIdleOperation::cancel() +{ + IMAPOperation::cancel(); + interruptIdle(); +} + void IMAPIdleOperation::interruptIdle() { + bool setupSuccess; + MCB_LOCK(&mLock); mInterrupted = true; + setupSuccess = mSetupSuccess; MCB_UNLOCK(&mLock); - if (mSetupSuccess) { + if (setupSuccess && session() != NULL) { session()->session()->interruptIdle(); } } - diff --git a/src/async/imap/MCIMAPIdleOperation.h b/src/async/imap/MCIMAPIdleOperation.h index 29bfe7400..bfd14bb5d 100644 --- a/src/async/imap/MCIMAPIdleOperation.h +++ b/src/async/imap/MCIMAPIdleOperation.h @@ -24,6 +24,7 @@ namespace mailcore { virtual void setLastKnownUID(uint32_t uid); virtual uint32_t lastKnownUID(); + virtual void cancel(); virtual void interruptIdle(); public: // subclass behavior @@ -37,6 +38,8 @@ namespace mailcore { void prepare(void * data); void unprepare(void * data); bool isInterrupted(); + void setSetupSuccess(bool setupSuccess); + bool setupSuccess(); }; } diff --git a/src/core/basetypes/MCBasicLock.h b/src/core/basetypes/MCBasicLock.h index 5d704f9ea..16dbeea48 100644 --- a/src/core/basetypes/MCBasicLock.h +++ b/src/core/basetypes/MCBasicLock.h @@ -13,6 +13,13 @@ #define MCB_LOCK(l) AcquireSRWLockExclusive(l) #define MCB_UNLOCK(l) ReleaseSRWLockExclusive(l) +#define MCB_COND_TYPE CONDITION_VARIABLE +#define MCB_COND_INIT(c) InitializeConditionVariable(c) +#define MCB_COND_DESTROY(c) +/* 0 = the lock is held exclusively, which is how MCB_LOCK takes it. */ +#define MCB_COND_WAIT(c, l) SleepConditionVariableSRW(c, l, INFINITE, 0) +#define MCB_COND_BROADCAST(c) WakeAllConditionVariable(c) + #else #include @@ -24,6 +31,12 @@ #define MCB_LOCK(l) pthread_mutex_lock(l) #define MCB_UNLOCK(l) pthread_mutex_unlock(l) +#define MCB_COND_TYPE pthread_cond_t +#define MCB_COND_INIT(c) pthread_cond_init(c, NULL) +#define MCB_COND_DESTROY(c) pthread_cond_destroy(c) +#define MCB_COND_WAIT(c, l) pthread_cond_wait(c, l) +#define MCB_COND_BROADCAST(c) pthread_cond_broadcast(c) + #endif #endif /* mailcore2_MCBasicLock_h */ diff --git a/src/core/imap/MCIMAPSession.cpp b/src/core/imap/MCIMAPSession.cpp index ab94490c5..bfc4359e7 100644 --- a/src/core/imap/MCIMAPSession.cpp +++ b/src/core/imap/MCIMAPSession.cpp @@ -418,6 +418,8 @@ void IMAPSession::init() mLastFetchedSequenceNumber = 0; mCurrentFolder = NULL; MCB_LOCK_INIT(&mIdleLock); + MCB_COND_INIT(&mIdleCond); + mIdleInProgress = false; mState = STATE_DISCONNECTED; mImap = NULL; mProgressCallback = NULL; @@ -452,6 +454,7 @@ IMAPSession::~IMAPSession() MC_SAFE_RELEASE(mWelcomeString); MC_SAFE_RELEASE(mDefaultNamespace); MC_SAFE_RELEASE(mCurrentFolder); + MCB_COND_DESTROY(&mIdleCond); MCB_LOCK_DESTROY(&mIdleLock); MCB_LOCK_DESTROY(&mConnectionLoggerLock); } @@ -650,6 +653,13 @@ void IMAPSession::unsetup() mailimap * imap; LOCK(); + while (mIdleInProgress) { + if (mImap != NULL && mImap->imap_stream != NULL) { + mailstream_interrupt_idle(mImap->imap_stream); + mailstream_cancel(mImap->imap_stream); + } + MCB_COND_WAIT(&mIdleCond, &mIdleLock); + } imap = mImap; mImap = NULL; mIdleEnabled = false; @@ -3626,9 +3636,9 @@ bool IMAPSession::setupIdle() { // main thread LOCK(); - bool canIdle = mIdleEnabled; - if (mIdleEnabled) { - mailstream_setup_idle(mImap->imap_stream); + bool canIdle = mIdleEnabled && mImap != NULL && mImap->imap_stream != NULL && !mIdleInProgress; + if (canIdle) { + canIdle = mailstream_setup_idle(mImap->imap_stream) == 0; } UNLOCK(); return canIdle; @@ -3637,6 +3647,7 @@ bool IMAPSession::setupIdle() void IMAPSession::idle(String * folder, uint32_t lastKnownUID, ErrorCode * pError) { int r; + mailimap * imap; // connection thread selectIfNeeded(folder, pError); @@ -3661,25 +3672,35 @@ void IMAPSession::idle(String * folder, uint32_t lastKnownUID, ErrorCode * pErro } } - r = mailimap_idle(mImap); + LOCK(); + if (mImap == NULL || mImap->imap_stream == NULL || mIdleInProgress) { + UNLOCK(); + * pError = ErrorIdle; + return; + } + imap = mImap; + mIdleInProgress = true; + UNLOCK(); + + r = mailimap_idle(imap); if (r == MAILIMAP_ERROR_STREAM) { mShouldDisconnect = true; * pError = ErrorConnection; - return; + goto cleanup; } else if (r == MAILIMAP_ERROR_PARSE) { mShouldDisconnect = true; * pError = ErrorParse; - return; + goto cleanup; } else if (hasError(r)) { * pError = ErrorIdle; - return; + goto cleanup; } - if (!mImap->imap_selection_info->sel_has_exists && !mImap->imap_selection_info->sel_has_recent) { + if (!imap->imap_selection_info->sel_has_exists && !imap->imap_selection_info->sel_has_recent) { int r; - r = mailstream_wait_idle(mImap->imap_stream, MAX_IDLE_DELAY); + r = mailstream_wait_idle(imap->imap_stream, MAX_IDLE_DELAY); switch (r) { case MAILSTREAM_IDLE_ERROR: case MAILSTREAM_IDLE_CANCELLED: @@ -3687,7 +3708,7 @@ void IMAPSession::idle(String * folder, uint32_t lastKnownUID, ErrorCode * pErro mShouldDisconnect = true; * pError = ErrorConnection; MCLog("error or cancelled"); - return; + goto cleanup; } case MAILSTREAM_IDLE_INTERRUPTED: MCLog("interrupted by user"); @@ -3704,29 +3725,35 @@ void IMAPSession::idle(String * folder, uint32_t lastKnownUID, ErrorCode * pErro MCLog("found info before idling"); } - r = mailimap_idle_done(mImap); + r = mailimap_idle_done(imap); if (r == MAILIMAP_ERROR_STREAM) { mShouldDisconnect = true; * pError = ErrorConnection; - return; + goto cleanup; } else if (r == MAILIMAP_ERROR_PARSE) { mShouldDisconnect = true; * pError = ErrorParse; - return; + goto cleanup; } else if (hasError(r)) { * pError = ErrorIdle; - return; + goto cleanup; } * pError = ErrorNone; + +cleanup: + LOCK(); + mIdleInProgress = false; + MCB_COND_BROADCAST(&mIdleCond); + UNLOCK(); } void IMAPSession::interruptIdle() { // main thread LOCK(); - if (mIdleEnabled) { + if (mIdleEnabled && mImap != NULL && mImap->imap_stream != NULL) { mailstream_interrupt_idle(mImap->imap_stream); } UNLOCK(); @@ -3736,7 +3763,13 @@ void IMAPSession::unsetupIdle() { // main thread LOCK(); - if (mIdleEnabled) { + while (mIdleInProgress) { + if (mImap != NULL && mImap->imap_stream != NULL) { + mailstream_interrupt_idle(mImap->imap_stream); + } + MCB_COND_WAIT(&mIdleCond, &mIdleLock); + } + if (mIdleEnabled && mImap != NULL && mImap->imap_stream != NULL) { mailstream_unsetup_idle(mImap->imap_stream); } UNLOCK(); @@ -3751,8 +3784,9 @@ void IMAPSession::interruptCurrentCommand() { // mailstream_cancel() must be called while holding the lock: unsetup() nils mImap under it and // frees the stream right after releasing it, so a pointer grabbed and used outside the lock - // would be a use-after-free. Holding it here is safe - mailstream_cancel() only takes the - // cancel object's own mutex and writes one byte to a pipe, it never blocks. + // would be a use-after-free. mailstream_cancel() itself never blocks - it takes the cancel + // object's own mutex and writes one byte to a pipe - but acquiring the lock can now wait out a + // teardown that is itself waiting for an IDLE to unwind, so this is no longer a bounded wait. LOCK(); if (mImap != NULL && mImap->imap_stream != NULL) { // Deliberately not raising mShouldDisconnect here: the command this cuts fails with a diff --git a/src/core/imap/MCIMAPSession.h b/src/core/imap/MCIMAPSession.h index 73f276faa..8663a76dc 100644 --- a/src/core/imap/MCIMAPSession.h +++ b/src/core/imap/MCIMAPSession.h @@ -316,6 +316,10 @@ namespace mailcore { unsigned int mLastFetchedSequenceNumber; String * mCurrentFolder; MCB_LOCK_TYPE mIdleLock; + // Signalled under mIdleLock when mIdleInProgress drops: teardown waits on it rather than + // freeing the stream idle() is still blocked on. + MCB_COND_TYPE mIdleCond; + bool mIdleInProgress; // Written on this session's own thread, read by IMAPAsyncSession's connection selection // through IMAPAsyncConnection::needsReconnect: atomic so that read is defined. // mShouldDisconnect has one more writer, scheduleReconnect(), on any thread. diff --git a/src/include/MailCore/MCBasicLock.h b/src/include/MailCore/MCBasicLock.h index 5d704f9ea..16dbeea48 100644 --- a/src/include/MailCore/MCBasicLock.h +++ b/src/include/MailCore/MCBasicLock.h @@ -13,6 +13,13 @@ #define MCB_LOCK(l) AcquireSRWLockExclusive(l) #define MCB_UNLOCK(l) ReleaseSRWLockExclusive(l) +#define MCB_COND_TYPE CONDITION_VARIABLE +#define MCB_COND_INIT(c) InitializeConditionVariable(c) +#define MCB_COND_DESTROY(c) +/* 0 = the lock is held exclusively, which is how MCB_LOCK takes it. */ +#define MCB_COND_WAIT(c, l) SleepConditionVariableSRW(c, l, INFINITE, 0) +#define MCB_COND_BROADCAST(c) WakeAllConditionVariable(c) + #else #include @@ -24,6 +31,12 @@ #define MCB_LOCK(l) pthread_mutex_lock(l) #define MCB_UNLOCK(l) pthread_mutex_unlock(l) +#define MCB_COND_TYPE pthread_cond_t +#define MCB_COND_INIT(c) pthread_cond_init(c, NULL) +#define MCB_COND_DESTROY(c) pthread_cond_destroy(c) +#define MCB_COND_WAIT(c, l) pthread_cond_wait(c, l) +#define MCB_COND_BROADCAST(c) pthread_cond_broadcast(c) + #endif #endif /* mailcore2_MCBasicLock_h */ diff --git a/src/include/MailCore/MCIMAPIdleOperation.h b/src/include/MailCore/MCIMAPIdleOperation.h index 29bfe7400..bfd14bb5d 100644 --- a/src/include/MailCore/MCIMAPIdleOperation.h +++ b/src/include/MailCore/MCIMAPIdleOperation.h @@ -24,6 +24,7 @@ namespace mailcore { virtual void setLastKnownUID(uint32_t uid); virtual uint32_t lastKnownUID(); + virtual void cancel(); virtual void interruptIdle(); public: // subclass behavior @@ -37,6 +38,8 @@ namespace mailcore { void prepare(void * data); void unprepare(void * data); bool isInterrupted(); + void setSetupSuccess(bool setupSuccess); + bool setupSuccess(); }; } diff --git a/src/include/MailCore/MCIMAPSession.h b/src/include/MailCore/MCIMAPSession.h index 73f276faa..8663a76dc 100644 --- a/src/include/MailCore/MCIMAPSession.h +++ b/src/include/MailCore/MCIMAPSession.h @@ -316,6 +316,10 @@ namespace mailcore { unsigned int mLastFetchedSequenceNumber; String * mCurrentFolder; MCB_LOCK_TYPE mIdleLock; + // Signalled under mIdleLock when mIdleInProgress drops: teardown waits on it rather than + // freeing the stream idle() is still blocked on. + MCB_COND_TYPE mIdleCond; + bool mIdleInProgress; // Written on this session's own thread, read by IMAPAsyncSession's connection selection // through IMAPAsyncConnection::needsReconnect: atomic so that read is defined. // mShouldDisconnect has one more writer, scheduleReconnect(), on any thread. diff --git a/unittest/IMAPIdleCancellationTests.swift b/unittest/IMAPIdleCancellationTests.swift index af31fc374..e00e11064 100644 --- a/unittest/IMAPIdleCancellationTests.swift +++ b/unittest/IMAPIdleCancellationTests.swift @@ -320,7 +320,7 @@ final class IMAPIdleCancellationTests: XCTestCase { runOffMainThread(timeout: 30) { let (operation, finished) = self.startIdle(session) - XCTAssertTrue(server.waitForIdleEntered(timeout: 5), "The session was expected to enter IDLE. Client sent:\n\(server.transcript)") + XCTAssertTrue(server.waitForIdleEntered(timeout: 20), "The session was expected to enter IDLE. Client sent:\n\(server.transcript)") XCTAssertEqual(finished.wait(timeout: .now() + 1), .timedOut, "IDLE was expected to still be running") operation.interruptIdle() @@ -332,9 +332,9 @@ final class IMAPIdleCancellationTests: XCTestCase { } /// Upstream's `testCancelWakesIdleIteration`: cancelling every operation on the session has to - /// wake a running IDLE. Upstream made this true by having IMAPIdleOperation::cancel() call - /// interruptIdle(); without that, cancel only flips a flag the blocked IDLE never looks at, and - /// everything queued behind it - a disconnect included - waits for the IDLE to time out. + /// wake a running IDLE. IMAPIdleOperation::cancel() calls interruptIdle() to make that true; + /// without it, cancel only flips a flag the blocked IDLE never looks at, and everything queued + /// behind it - a disconnect included - waits for the IDLE to time out. func testCancelAllOperationsWakesRunningIdle() throws { let server = try FakeIdleIMAPServer() defer { server.stop() } @@ -342,28 +342,46 @@ final class IMAPIdleCancellationTests: XCTestCase { let session = makeSession(port: server.port) runOffMainThread(timeout: 30) { - let (operation, finished) = self.startIdle(session) + let (_, finished) = self.startIdle(session) - XCTAssertTrue(server.waitForIdleEntered(timeout: 5), "The session was expected to enter IDLE. Client sent:\n\(server.transcript)") + XCTAssertTrue(server.waitForIdleEntered(timeout: 20), "The session was expected to enter IDLE. Client sent:\n\(server.transcript)") XCTAssertEqual(finished.wait(timeout: .now() + 1), .timedOut, "IDLE was expected to still be running") session.cancelAllOperations() - let stopped = self.waitUntilQueueStopped(session, timeout: 5) - // Known gap until upstream 03a19472 ("Fix IMAP IDLE teardown races") is merged: the - // expectation is strict, so the test fails the day the fix lands and this block must go. - // Spark does not depend on it today - it never calls cancelAllOperations() on an IMAP - // session and always interruptIdle()s before disconnecting the idle session. - XCTExpectFailure("IMAPIdleOperation::cancel() does not interrupt a running IDLE yet") { - XCTAssertTrue(stopped, "cancelAllOperations() did not wake the running IDLE") - } - if stopped { - XCTAssertTrue(server.waitForDoneOrClose(timeout: 5), "The server saw neither DONE nor a close") - } - else { - // Do not leave the IDLE blocked behind us: end it the way that is known to work. - operation.interruptIdle() - _ = self.waitUntilQueueStopped(session, timeout: 5) + XCTAssertTrue(self.waitUntilQueueStopped(session, timeout: 5), + "cancelAllOperations() did not wake the running IDLE") + XCTAssertTrue(server.waitForDoneOrClose(timeout: 5), "The server saw neither DONE nor a close") + } + } + + /// Upstream ships a C++ stress harness for this (`tests/test-imap-idle.cpp`, commit ab53363b) + /// because it has no Swift tests; our CMake `tests` executable is not what CI runs, so its third + /// goal - "the process does not crash or hang under repetition, especially under ASan" - is + /// covered here instead. The case above proves the cancel wakes one IDLE; this one repeats the + /// whole connect/idle/cancel/tear-down cycle so a leak or a stale stream has somewhere to show. + func testRepeatedCancelDuringIdleDoesNotHangOrCrash() throws { + for iteration in 0 ..< 6 { + let server = try FakeIdleIMAPServer() + defer { server.stop() } + + // Scoped so the session is released before the next iteration builds another one. + let session = makeSession(port: server.port) + + runOffMainThread(timeout: 30) { + _ = self.startIdle(session) + + // Gate on the server rather than on a sleep: a cancel that arrives before IDLE is + // established is dropped by the operation queue and proves nothing. + XCTAssertTrue(server.waitForIdleEntered(timeout: 20), + "Iteration \(iteration): the session was expected to enter IDLE. Client sent:\n\(server.transcript)") + + session.cancelAllOperations() + + // A cancelled operation is not required to report completion, so the stopped queue + // is what says the IDLE let go of the connection. + XCTAssertTrue(self.waitUntilQueueStopped(session, timeout: 10), + "Iteration \(iteration): the operation queue did not stop after the cancel") } } } From 8b0bf9fde048e13b96c57a87338f7b56d52a1c05 Mon Sep 17 00:00:00 2001 From: Dmytro Bezverkhnii Date: Wed, 16 Sep 2026 11:23:07 +0300 Subject: [PATCH 2/2] Test: give the IDLE test bodies a budget that clears their waits COR-221 Raising the waitForIdleEntered precondition to 20s left the enclosing runOffMainThread() at 30s, which the sum of the waits inside a body can now exceed - so a merely slow but correct run would have failed on the wrapper instead of on its own assertion, trading one flake for another. Co-Authored-By: Claude Opus 5 --- unittest/IMAPIdleCancellationTests.swift | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/unittest/IMAPIdleCancellationTests.swift b/unittest/IMAPIdleCancellationTests.swift index e00e11064..681e591b7 100644 --- a/unittest/IMAPIdleCancellationTests.swift +++ b/unittest/IMAPIdleCancellationTests.swift @@ -273,6 +273,8 @@ final class IMAPIdleCancellationTests: XCTestCase { } /// Runs the test body off the main thread while the main thread keeps spinning its run loop. + /// The budget has to clear the sum of the waits inside the body, not just the longest one: a + /// body that is merely slow must fail on its own assertion, not on this wrapper. /// mailcore hands parts of an operation's lifecycle to the main queue and waits for them, so a /// test that blocks the main thread never gets its operation started in the first place. private func runOffMainThread(timeout: TimeInterval, _ body: @escaping () -> Void) { @@ -317,7 +319,7 @@ final class IMAPIdleCancellationTests: XCTestCase { let session = makeSession(port: server.port) - runOffMainThread(timeout: 30) { + runOffMainThread(timeout: 60) { let (operation, finished) = self.startIdle(session) XCTAssertTrue(server.waitForIdleEntered(timeout: 20), "The session was expected to enter IDLE. Client sent:\n\(server.transcript)") @@ -341,7 +343,7 @@ final class IMAPIdleCancellationTests: XCTestCase { let session = makeSession(port: server.port) - runOffMainThread(timeout: 30) { + runOffMainThread(timeout: 60) { let (_, finished) = self.startIdle(session) XCTAssertTrue(server.waitForIdleEntered(timeout: 20), "The session was expected to enter IDLE. Client sent:\n\(server.transcript)") @@ -368,7 +370,7 @@ final class IMAPIdleCancellationTests: XCTestCase { // Scoped so the session is released before the next iteration builds another one. let session = makeSession(port: server.port) - runOffMainThread(timeout: 30) { + runOffMainThread(timeout: 60) { _ = self.startIdle(session) // Gate on the server rather than on a sleep: a cancel that arrives before IDLE is