COR-211, COR-212, COR-213, COR-216 - Fix IMAP login and connection lifetime crashes

Package.swift

@@ -265,7 +265,7 @@ var targets: [Target] = [
             "unittest.cpp",
             "unittest.mm"
         ],
-        sources: ["CertificateUtilsTests.swift", "IMAPConnectionLeaseTests.swift", "IMAPIdleCancellationTests.swift", "IMAPInterruptCurrentCommandTests.swift", "LeaseTestTCPEndpoint.swift", "LibetpanHelperTests.swift", "unittest.swift"],
+        sources: ["CertificateUtilsTests.swift", "IMAPConnectionLeaseTests.swift", "IMAPConnectionOwnerLifetimeTests.swift", "IMAPIdleCancellationTests.swift", "IMAPInterruptCurrentCommandTests.swift", "IMAPLoginTests.swift", "LeaseTestTCPEndpoint.swift", "LibetpanHelperTests.swift", "unittest.swift"],
         resources: [
             .copy("data")
         ]

src/async/imap/MCIMAPAsyncConnection.cpp

@@ -277,6 +277,10 @@ IMAPOperation * IMAPAsyncConnection::disconnectOperation()
 {
     IMAPDisconnectOperation * op = new IMAPDisconnectOperation();
     op->setSession(this);
+    // Retains the owner like every operation the owner creates: this connection reaches it through
+    // a raw pointer on each queue start and stop, and the queue's own retain is released before
+    // a queued operation restarts it.
+    op->setMainSession(mOwner);
     op->autorelease();
     return op;
 }
@@ -296,6 +300,11 @@ bool IMAPAsyncConnection::needsReconnect()
     return mSession->needsReconnect();
 }
 
+void IMAPAsyncConnection::scheduleReconnect()
+{
+    mSession->scheduleReconnect();
+}
+
 unsigned int IMAPAsyncConnection::operationsCount()
 {
     return mQueue->count();

src/async/imap/MCIMAPAsyncConnection.h

@@ -174,6 +174,9 @@ namespace mailcore {
         // (see IMAPSession::needsReconnect). Declared last on purpose: this class is exported, and
         // a virtual inserted among the existing ones would shift every vtable slot after it.
         virtual bool needsReconnect();
+        // Makes the next command on this connection rebuild it first; same contract as
+        // IMAPSession::scheduleReconnect(). Declared last, like needsReconnect().
+        virtual void scheduleReconnect();
     };
 
 }

src/async/imap/MCIMAPOperation.h

@@ -50,9 +50,11 @@ namespace mailcore {
          the operations queued behind it (a disconnect, most importantly) run immediately.
          Does nothing when the operation is not the one running.
 
-         Teardown of this connection only - it is left unusable and reconnects on next use, so call
-         it for a command that is being abandoned (cancelled, or given up on), never to hurry up a
-         command whose result still matters.
+         Teardown of this connection only - the cut command fails with a connection error and the
+         connection is rebuilt before the one after; landing with no command on the wire, it is the
+         next command that fails and the one after that rebuilds - so call it for a command that is
+         being abandoned (cancelled, or given up on), never to hurry up a command whose result still
+         matters.
          Returns whether this operation was the one the queue was running at that moment. That may
          include a command that finished just as the interrupt landed: its result is intact, but
          the stream is cancelled all the same. */

src/c/imap/CIMAPAsyncConnection.cpp

@@ -16,3 +16,4 @@ C_SYNTHESIZE_FUNC_WITH_SCALAR(unsigned int, leaseGeneration)
 C_SYNTHESIZE_FUNC_WITH_SCALAR(unsigned int, operationsCount)
 C_SYNTHESIZE_FUNC_WITH_SCALAR(double, lastLoginTime)
 C_SYNTHESIZE_FUNC_WITH_OBJ(CIMAPBaseOperation, disconnectOperation)
+C_SYNTHESIZE_FUNC_WITH_VOID(scheduleReconnect)

src/c/imap/CIMAPAsyncConnection.h

@@ -25,6 +25,7 @@ extern "C" {
     C_SYNTHESIZE_READONLY_PROPERTY_DEFINITION(CIMAPAsyncConnection, double, lastLoginTime)
 
     C_SYNTHESIZE_FUNC_DEFINITION(CIMAPAsyncConnection, CIMAPBaseOperation, disconnectOperation)
+    C_SYNTHESIZE_FUNC_DEFINITION(CIMAPAsyncConnection, void, scheduleReconnect)
 
 #ifdef __cplusplus
 }

src/core/imap/MCIMAPSession.cpp

@@ -1116,6 +1116,15 @@ void IMAPSession::login(ErrorCode * pError)
     else {
         // TODO: namespace should be shared with other sessions for non automatic namespace.
     }
+
+    // identity() starts with connectIfNeeded(): with mShouldDisconnect raised meanwhile it tears the
+    // connection down and rebuilds it, and its result is ignored above. A rebuilt connection is not
+    // logged in - and a rebuild that failed has no mImap at all - so this must not read as a
+    // successful login; nor may one whose ignored ID left the stream marked for teardown.
+    if (mState != STATE_LOGGEDIN || mShouldDisconnect) {
+        * pError = ErrorConnection;
+        return;
+    }
 
     mAutomaticConfigurationDone = true;
 
@@ -3747,11 +3756,10 @@ void IMAPSession::interruptCurrentCommand()
     LOCK();
     if (mImap != NULL && mImap->imap_stream != NULL) {
         mailstream_cancel(mImap->imap_stream);
-        // libetpan never clears a stream's cancelled state: every read and write on it fails from
-        // here on. The next command must reconnect, and connectIfNeeded does that for this flag,
-        // so the connection stays pooled and heals on its own instead of relying on the caller
-        // to tear it down.
-        mShouldDisconnect = true;
+        // Deliberately not raising mShouldDisconnect here: the command this cuts fails with a
+        // stream error and raises it itself, at a point its caller checks. Raised from this thread
+        // it can land between two commands of one login(), where the nested connectIfNeeded()
+        // would rebuild the connection under a caller that ignores the result.
     }
     UNLOCK();
 }
@@ -4432,6 +4440,11 @@ bool IMAPSession::needsReconnect()
     return mState == STATE_DISCONNECTED || mShouldDisconnect;
 }
 
+void IMAPSession::scheduleReconnect()
+{
+    mShouldDisconnect = true;
+}
+
 double IMAPSession::lastLoginTime()
 {
     LOCK();

src/core/imap/MCIMAPSession.h

@@ -260,6 +260,11 @@ namespace mailcore {
         // Declared last: this class is exported, and a virtual inserted among the existing ones
         // would shift every vtable slot after it.
         virtual bool needsReconnect();
+        // Makes the next command rebuild the connection first, as a failed command would have.
+        // Any thread, but between operations only: raised while one runs, it is met by the next
+        // connectIfNeeded() inside that operation - login() then fails with ErrorConnection.
+        // Declared last, for the same reason as needsReconnect().
+        virtual void scheduleReconnect();
 
     private:
         String * mHostname;
@@ -309,6 +314,7 @@ namespace mailcore {
 		MCB_LOCK_TYPE mIdleLock;
         // 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.
         std::atomic<int> mState;
         double mLastLoginTime;
         mailimap * mImap;

src/include/MailCore/CIMAPAsyncConnection.h

@@ -25,6 +25,7 @@ extern "C" {
     C_SYNTHESIZE_READONLY_PROPERTY_DEFINITION(CIMAPAsyncConnection, double, lastLoginTime)
 
     C_SYNTHESIZE_FUNC_DEFINITION(CIMAPAsyncConnection, CIMAPBaseOperation, disconnectOperation)
+    C_SYNTHESIZE_FUNC_DEFINITION(CIMAPAsyncConnection, void, scheduleReconnect)
 
 #ifdef __cplusplus
 }

src/include/MailCore/MCIMAPAsyncConnection.h

@@ -174,6 +174,9 @@ namespace mailcore {
         // (see IMAPSession::needsReconnect). Declared last on purpose: this class is exported, and
         // a virtual inserted among the existing ones would shift every vtable slot after it.
         virtual bool needsReconnect();
+        // Makes the next command on this connection rebuild it first; same contract as
+        // IMAPSession::scheduleReconnect(). Declared last, like needsReconnect().
+        virtual void scheduleReconnect();
     };
 
 }

src/include/MailCore/MCIMAPOperation.h

@@ -50,9 +50,11 @@ namespace mailcore {
          the operations queued behind it (a disconnect, most importantly) run immediately.
          Does nothing when the operation is not the one running.
 
-         Teardown of this connection only - it is left unusable and reconnects on next use, so call
-         it for a command that is being abandoned (cancelled, or given up on), never to hurry up a
-         command whose result still matters.
+         Teardown of this connection only - the cut command fails with a connection error and the
+         connection is rebuilt before the one after; landing with no command on the wire, it is the
+         next command that fails and the one after that rebuilds - so call it for a command that is
+         being abandoned (cancelled, or given up on), never to hurry up a command whose result still
+         matters.
          Returns whether this operation was the one the queue was running at that moment. That may
          include a command that finished just as the interrupt landed: its result is intact, but
          the stream is cancelled all the same. */

src/include/MailCore/MCIMAPSession.h

@@ -260,6 +260,11 @@ namespace mailcore {
         // Declared last: this class is exported, and a virtual inserted among the existing ones
         // would shift every vtable slot after it.
         virtual bool needsReconnect();
+        // Makes the next command rebuild the connection first, as a failed command would have.
+        // Any thread, but between operations only: raised while one runs, it is met by the next
+        // connectIfNeeded() inside that operation - login() then fails with ErrorConnection.
+        // Declared last, for the same reason as needsReconnect().
+        virtual void scheduleReconnect();
 
     private:
         String * mHostname;
@@ -309,6 +314,7 @@ namespace mailcore {
 		MCB_LOCK_TYPE mIdleLock;
         // 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.
         std::atomic<int> mState;
         double mLastLoginTime;
         mailimap * mImap;

src/swift/imap/IMAPAsyncConnection.swift

@@ -1,3 +1,4 @@
+import Dispatch
 import Foundation
 import CMailCore
 
@@ -38,12 +39,27 @@ public class MCOIMAPAsyncConnection: NSObjectCompat {
     /// Returns the lease if its holder never did. A leaked lease is permanent otherwise — nothing
     /// in the pool clears a reservation on its own — and the connection would be lost to the pool
     /// for the life of the session. Torn down rather than pooled: a holder that lost track of its
-    /// lease cannot have left the connection in a state anybody should inherit. Best effort:
-    /// deinit runs on whatever thread drops the last reference, outside the serialisation the
-    /// release contract asks for; a holder that releases explicitly never gets here.
+    /// lease cannot have left the connection in a state anybody should inherit. deinit runs on
+    /// whatever thread drops the last reference, so the release hops to the session's queue: a
+    /// release starts the teardown operation, and every operation start on a session must come
+    /// from that queue (see MCOIMAPSession.acquireConnection). The lease is therefore back in the
+    /// pool only once that queue has run, not the instant the handle dies. A holder that releases
+    /// explicitly never gets here.
     deinit {
-        session.releaseConnection(self, disconnect: true)
-        connection.release()
+        let session = self.session
+        let connection = self.connection
+        let leaseGeneration = self.leaseGeneration
+        (session.dispatchQueue ?? DispatchQueue.main).async {
+            session.releaseConnection(connection, leaseGeneration: leaseGeneration, disconnect: true)
+            connection.release()
+        }
+    }
+
+    /// Makes the next command on this connection rebuild it first, the way a failed command would
+    /// have. For tests: raises the flag from outside the connection's thread without cutting the
+    /// stream, so a command in flight completes and the flag is met by the next one.
+    internal func scheduleReconnect() {
+        connection.scheduleReconnect()
     }
 
     internal var isReserved: Bool {

src/swift/imap/IMAPBaseOperation.swift

@@ -34,9 +34,10 @@ public class MCOIMAPBaseOperation : MCOOperation {
      the one running.
 
      Unlike cancel(), which only raises a flag mailcore checks before starting an operation, this
-     reaches the command already in flight. It costs the connection: the stream stays cancelled and
-     is rebuilt on next use, so call it for a command being abandoned, never to hurry up one whose
-     result still matters.
+     reaches the command already in flight. It costs the connection: the cut command fails with a
+     connection error and the connection is rebuilt before the one after (landing with no command on
+     the wire, it is the next command that fails and the one after that rebuilds), so call it for a
+     command being abandoned, never to hurry up one whose result still matters.
 
      - Returns: whether this operation was the one the queue was running at that moment. That may
      include a command that finished just as the interrupt landed: its result is intact, but the

src/swift/imap/IMAPSession.swift

@@ -194,8 +194,12 @@ public class MCOIMAPSession: NSObjectCompat {
      contract as acquireConnection(folder:).
      */
     public func releaseConnection(_ connection: MCOIMAPAsyncConnection, disconnect: Bool) {
+        releaseConnection(connection.connection, leaseGeneration: connection.leaseGeneration, disconnect: disconnect)
+    }
+
+    internal func releaseConnection(_ connection: CIMAPAsyncConnection, leaseGeneration: UInt32, disconnect: Bool) {
         mailCoreAutoreleasePool {
-            session.releaseConnection(connection.connection, connection.leaseGeneration, disconnect)
+            session.releaseConnection(connection, leaseGeneration, disconnect)
         }
     }
 

unittest/IMAPConnectionLeaseTests.swift

@@ -136,8 +136,8 @@ final class IMAPConnectionLeaseTests: XCTestCase {
 
     func testInterruptedConnectionReconnectsOnItsNextCommand() throws {
         // LOGIN and what mailcore sends right after it (CAPABILITY, the delimiter LIST) are
-        // answered, so that the command the interrupt cuts is the NOOP itself: a stream error
-        // inside any of those already schedules a reconnect on its own, one inside NOOP does not.
+        // answered, so that the command the interrupt cuts is the NOOP itself - a plain command
+        // outside login(), whose stream error is what has to schedule the reconnect.
         let endpoint = try LeaseTestTCPEndpoint(greeting: "* OK [CAPABILITY IMAP4rev1] LeaseTestTCPEndpoint ready\r\n",
                                                 answers: ["LOGIN": "",
                                                           "CAPABILITY": "* CAPABILITY IMAP4rev1\r\n",
@@ -434,7 +434,15 @@ final class IMAPConnectionLeaseTests: XCTestCase {
             XCTAssertNil(session.acquireConnection(folder: nil), "the only connection is leased")
         }
 
-        guard let reacquired = session.acquireConnection(folder: nil) else {
+        // The dropped handle returns its lease on the session's queue - the main queue here -
+        // so the pool frees up once that queue has run.
+        var reacquired: MCOIMAPAsyncConnection?
+        let deadline = Date(timeIntervalSinceNow: 5)
+        while reacquired == nil && Date() < deadline {
+            RunLoop.main.run(until: Date(timeIntervalSinceNow: 0.02))
+            reacquired = session.acquireConnection(folder: nil)
+        }
+        guard let reacquired = reacquired else {
             return XCTFail("A dropped handle must have returned its lease to the pool")
         }
         session.releaseConnection(reacquired, disconnect: false)

unittest/IMAPConnectionOwnerLifetimeTests.swift

@@ -0,0 +1,91 @@
+//
+//  IMAPConnectionOwnerLifetimeTests.swift
+//  mailcore2
+//
+//  A pooled connection must not outlive its IMAPAsyncSession while it still has work: the
+//  connection reaches its owner through a raw pointer on every queue start and stop.
+//
+
+#if canImport(Darwin)
+
+import Darwin
+import Dispatch
+import Foundation
+import XCTest
+
+#if SWIFT_PACKAGE
+import CMailCore
+#endif
+
+@testable import MailCore
+
+final class IMAPConnectionOwnerLifetimeTests: XCTestCase {
+
+    private func makeSession() -> MCOIMAPSession {
+        let session = MCOIMAPSession()
+        session.hostname = "127.0.0.1"
+        session.port = 1 // never connected: a disconnect on a fresh connection is a no-op
+        session.connectionType = ConnectionTypeClear
+        session.username = "user"
+        session.password = "password"
+        session.maximumConnections = 1
+        return session
+    }
+
+    private func runOffMainThread(timeout: TimeInterval, _ body: @escaping () -> Void) {
+        let finished = expectation(description: "test body")
+        DispatchQueue.global(qos: .userInitiated).async {
+            body()
+            finished.fulfill()
+        }
+        waitForExpectations(timeout: timeout)
+    }
+
+    private func start(_ operation: MCOIMAPOperation) -> DispatchSemaphore {
+        let finished = DispatchSemaphore(value: 0)
+        operation.start { _ in
+            finished.signal()
+        }
+        return finished
+    }
+
+    /// A connection-scoped disconnect is the one operation that does not retain the session it
+    /// belongs to. Queue one, let every other reference to the session go, wait for the
+    /// connection's queue thread to stop - its stop releases the session's last retain - and then
+    /// start the queued operation: the queue restarts and reports to an owner that no longer exists.
+    func testQueuedDisconnectKeepsTheOwnerAlive() {
+        var session: MCOIMAPSession? = makeSession()
+        var handle = session!.acquireConnection(folder: nil)
+        XCTAssertNotNil(handle)
+
+        // Retains the connection (through the operation), not the session.
+        let later = handle!.disconnectOperation()
+
+        runOffMainThread(timeout: 30) {
+            // Wakes the connection's queue thread; when it stops it releases its retain of the
+            // session, which is the last one once the Swift wrappers below are gone.
+            XCTAssertEqual(self.start(handle!.disconnectOperation()).wait(timeout: .now() + 5), .success)
+
+            session!.releaseConnection(handle!, disconnect: false)
+
+            // The queue thread lingers for about a second after its last operation and releases
+            // its retain of the session when it stops; only then are the wrappers below the last
+            // holders.
+            let deadline = Date(timeIntervalSinceNow: 10)
+            while session!.isOperationQueueRunning && Date() < deadline {
+                usleep(20_000)
+            }
+            XCTAssertFalse(session!.isOperationQueueRunning, "the connection's queue was expected to stop")
+            handle = nil
+            // The dropped handle returns its lease on the session's queue (the main queue here)
+            // and holds the session until then; let that run before the session is dropped.
+            DispatchQueue.main.sync {}
+            session = nil
+
+            XCTAssertEqual(self.start(later).wait(timeout: .now() + 5), .success,
+                           "The queued disconnect must still run once the pool has been dropped")
+        }
+    }
+}
+
+#endif