[dev] [PATCH] ActiveSync: Fix email sync error on corner case

[Michael J Rubinsky]

Quoting Michael J Rubinsky <mrubinsk at horde.org>:

> Quoting Thomas Jarosch <thomas.jarosch at intra2net.com>:
>
>> Hi Michael,
>>
>> On Thursday, 12. December 2013 12:18:01 Michael J Rubinsky wrote:
>>>> Sounds like a possibility. ActiveSync_State_Base::getChanges() calls
>>>> getServerChanges(). If we are not pinging the folder, we call
>>>> $folder->updateState() later on.
>>>>
>>>> When an Horde_Exception is thrown in getServerChanges(),
>>>> we just catch it and return an empty changes array.
>>>> Still we are executing $folder->updateState().
>>>>
>>>> What about some kind of race condition: A new message / just deleted
>>>> message is not listed in $this->_messages, but part of the $messages
>>>> list given to setChanges(). We'll then throw the exception and kill the
>>>> flags
>>>> later on in updateState().
>>>
>>> If it's a new message, the UID would be greater than the previous
>>> UIDNEXT value, so it would be treated as a new message and added to
>>> the $_messages array. Even if it was removed in the small time between
>>> we received the message list and called setChanges(), it
>>> wouldn't/shouldn't matter. The only way I can see this happening is if
>>> the UIDNEXT values was incorrect, or somehow a message with a UID
>>> lower than the already existing lowest on the device is provided to
>>> setChanges().
>>
>> ok, makes sense. I've seen you already changed the code a bit
>> to be more gentle to errors.
>>
>> What I wanted to test in a quiet minute (with the old code):
>> Throw an exception on purpose in getServerChanges().

Now that I'm home, I looked at this some more. I added some catch  
blocks that were missing in  
Horde_ActiveSync_Imap_Adapter::getMessageChanges(), but that shouldn't  
have had any effect anyway (since the exception would still have been  
caught in Horde_Core_ActiveSync_Driver::getServerChanges() anyway.  
Additionally, and exception thrown from getServerChanges would NOT be  
caught in H_A_State_Base::getChanges() so would percolate up, thus  
preventing $folder->updateState() from being called and triggering the  
bug.

In other words, I think we are good :)



>> Something like this (pseudo code only):
>> ---------------------
>> clearstatcache();
>> if (file_exists("/tmp/throw_exception_once")) {
>>   unlink("/tmp/throw_exception_once");
>>   throw new Horde_Exception("fake IMAP error");
>> }
>> ---------------------
>>
>>
>> I want to find out if any thrown exception inside getServerChanges()
>> kills the flag array on non-CONDSTORE servers as suspected
>> later on in $folder->updateState().
>
> Basically, this issue occurs when updateState() is called without  
> the flag array being set. It looks like the only way this can happen  
> (now that the previous issue was fixed) is if $folder->setChanges()  
> is not called, but $folder->updateState() is. In the case you  
> describe above, it would only be a problem if the exception thrown  
> from inside Horde_Core_ActiveSync_Driver::getServerChanges() is  
> thrown before $this->_imap->getMessageChanges() is called - since  
> that is where $folder->setChanges() is called. Also, it looks like  
> we should probably catch exceptions from the imap library in  
> Horde_ActiveSync_Imap_Adapter::getMessageChanges() to ensure that  
> doesn't prevent it from being called.
>
> Either way, it wouldn't hurt to add some logic to  
> Horde_ActiveSync_Folder_Imap to ensure that $flags was set before  
> mangling $_messages.
>
> $todo++;
>
>
> -- 
> mike
>
> The Horde Project (www.horde.org)
> mrubinsk at horde.org


-- 
mike

The Horde Project (www.horde.org)
mrubinsk at horde.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 5849 bytes
Desc: S/MIME Signature
URL: <http://lists.horde.org/archives/dev/attachments/20131220/bfaaf6bb/attachment.bin>