Merge pull request #113 from readdle/fix/COR-211-imap-login-postcondition

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

Author: dbezverkhnii

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();
     };
 
 }