COR-221 - Wait out a running IDLE before freeing its stream

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

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

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 <pthread.h>
@@ -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 */

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,33 +3672,43 @@ 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:
             {
                 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

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.

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 <pthread.h>
@@ -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 */

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

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.