6.0.0-git
2019-03-24

[#12097] Export of mailfolders fails with cyrus imap
Summary Export of mailfolders fails with cyrus imap
Queue Horde Groupware Webmail Edition
Queue Version 5.0.4
Type Bug
State Resolved
Priority 1. Low
Owners slusarz (at) horde (dot) org
Requester roderick.braun (at) ph-freiburg (dot) de
Created 2013-03-06 (2209 days ago)
Due
Updated 2013-03-08 (2207 days ago)
Assigned 2013-03-07 (2208 days ago)
Resolved 2013-03-08 (2207 days ago)
Milestone
Patch Yes

History
2013-03-08 21:58:25 Michael Slusarz Comment #9
State ⇒ Resolved
Reply to this comment
1 fix in Horde_Imap_Client 2.7.2 (+1 enhancement; +1 possible future 
enhancement):

Fix: Don't cache unless we have UID information.

1 enhancement: if caching, we should always send UID FETCH command to 
cache the maximum amount of data.

1 possible future enhancement: probably will be useful for user code 
to indicate a "one-time only" type query where they don't want results 
cached.  This should be implemented as an option to the fetch() command.
2013-03-08 21:52:44 Git Commit Comment #8 Reply to this comment
Changes have been made in Git (master):

commit e86598a5bb3f2f26ca0dafeeec3e9f26fab98698
Author: Michael M Slusarz <slusarz@horde.org>
Date:   Fri Mar 8 14:29:59 2013 -0700

     [mms] Don't cache FETCH data if it does not contain UID 
information (Bug #12097).

  .../Imap_Client/lib/Horde/Imap/Client/Base.php     |   12 +++++++++---
  framework/Imap_Client/package.xml                  |    4 ++--
  2 files changed, 11 insertions(+), 5 deletions(-)

http://git.horde.org/horde-git/-/commit/e86598a5bb3f2f26ca0dafeeec3e9f26fab98698
2013-03-08 14:46:57 roderick (dot) braun (at) ph-freiburg (dot) de Comment #7 Reply to this comment
I'm now thinking that this *could* happen if the IMAP server 
randomly throws in a non-UID FETCH response to some FETCH command 
that otherwise returns UIDs.  But I'd like to see IMAP log proof to 
be sure of this.  (And would probably explain why I have never seen 
this since I use Dovecot, not Cyrus).
Here is an example IMAP dialog of the "export mailbox" task. As you 
see there is no UID tag in the fetch response which is a rfc conform 
IMAP reply to "Fetch xxx (RFC822.SIZE)".

--- SNIP ---
<1362737883<3 Status {10+}
INBOX.test (MESSAGES)
1362737883>* STATUS INBOX.test (MESSAGES 1)
3 OK Completed
<1362737883<N01 NOOP
1362737883>N01 OK Completed
<1362737883<PROXY0 Enable Qresync
1362737883>* ENABLED CONDSTORE QRESYNC
PROXY0 OK Completed
<1362737883<4 Examine {10+}
INBOX.test
1362737883>* 1 EXISTS
* 0 RECENT
* FLAGS (\Answered \Flagged \Draft \Deleted \Seen)
* OK [PERMANENTFLAGS ()] Ok
* OK [UIDVALIDITY 1362736459] Ok
* OK [UIDNEXT 2] Ok
* OK [HIGHESTMODSEQ 2] Ok
* OK [URLMECH INTERNAL] Ok
4 OK [READ-ONLY] Completed
<1362737883<5 UID Search RETURN (ALL) 1:*
1362737883>* ESEARCH (TAG "5") UID ALL 1
5 OK Completed (1 msgs in 0.000 secs)
<1362737883<6 Fetch 1 (RFC822.SIZE)
1362737883>* 1 FETCH (RFC822.SIZE 1554)
6 OK Completed (0.000 sec)
<1362737883<PROXY1 Noop
1362737883>PROXY1 OK Completed
<1362737883<Q01 LOGOUT
1362737883>* BYE LOGOUT received
Q01 OK Completed

--- SNAP ---
2013-03-07 21:18:10 Michael Slusarz Comment #6 Reply to this comment
We are getting closer.  The problem is that _updateCache() should 
never be called unless we have UIDs.  We can't cache anything with 
sequence numbers, so if UIDs don't exist we shouldn't even reach 
this point.
I'm now thinking that this *could* happen if the IMAP server randomly 
throws in a non-UID FETCH response to some FETCH command that 
otherwise returns UIDs.  But I'd like to see IMAP log proof to be sure 
of this.  (And would probably explain why I have never seen this since 
I use Dovecot, not Cyrus).
2013-03-07 21:04:14 Michael Slusarz Comment #5 Reply to this comment
You are right it doesn't fix the problem - I traced the bug and 
found a problem in the method "_updateCache" in class 
"Horde_Imap_Client_Base".  The mapping array is created unchecked 
using the getUid() which is either an integer or "null".
We are getting closer.  The problem is that _updateCache() should 
never be called unless we have UIDs.  We can't cache anything with 
sequence numbers, so if UIDs don't exist we shouldn't even reach this 
point.

So it would be great if I could get either a backtrace at the point 
where getUid() returns null (via Horde::debug() - see below) and/or 
the IMAP log of the activity that causes this issue (also see below).



Horde::debug() documentation: http://wiki.horde.org/Doc/Dev/DebugH4





To further debug this issue, we need details of the IMP -> IMAP/POP 
communication.

To enable debugging, see instructions contained in 
imp/config/backends.php (the 'debug' config parameter).

Debugging should not be enabled on a production server,   Attach/post 
only the portion of the log that directly deals with the problem 
reported (it may be simplest to clear the log file and then perform 
the event that causes the error).
2013-03-07 20:58:10 Michael Slusarz Comment #4
Assigned to Michael Slusarz
State ⇒ Assigned
Reply to this comment
See also (possibly) Ticket #12031
You may say: This is also not solving the origin problem but  I 
think as long as you cannot be sure the method returns integer 
values in all cases you have to check the result in calling processes.
I disagree.

For a general purpose API call that is exposed to any users (in other 
words, where you don't control the calling code) it may make sense to 
be super liberal in what you accept.

But for a specific API/function call that really is only meant to be 
called within the library, this is a bad idea because:

1) You control the input and should guarantee the data is correct here
2) Because if you cant guarantee #1, that means there is a bug in our 
code that needs to be fixed (and may be causing issues elsewhere)
3) You are just adding complexity and performance hits when not necessary
2013-03-07 16:20:58 roderick (dot) braun (at) ph-freiburg (dot) de Comment #3
New Attachment: imap-update-mapping-fix2.patch Download
Reply to this comment
The problem is in the method "update" in .../Imap/Client/Ids/Map.php
which is not checking whether the mapping array contains only valid
values (I guess they should always be int?).
Yes.  So the question is... WHY are they not all integers for you?
I doubt this is only  a problem of our setup because looking in my 
IMAP logs I can't find any non rfc answer on the server side which 
could explain the problem.
Your patch is not useful because it masks the problem but doesn't fix it.
You are right it doesn't fix the problem - I traced the bug and found 
a problem in the method "_updateCache" in class 
"Horde_Imap_Client_Base".  The mapping array is created unchecked 
using the getUid() which is either an integer or "null". So I would 
suggest to check the retrieved uid before assigning it to the mapping 
array.

Again I attached a patch.

You may say: This is also not solving the origin problem but  I think 
as long as you cannot be sure the method returns integer values in all 
cases you have to check the result in calling processes.
(Btw: It is not nescessary to update the cached mapping anyway to 
export mails successfully.)


2013-03-07 02:10:15 Michael Slusarz Comment #2
State ⇒ Feedback
Reply to this comment
The problem is in the method "update" in .../Imap/Client/Ids/Map.php 
which is not checking whether the mapping array contains only valid 
values (I guess they should always be int?).
Yes.  So the question is... WHY are they not all integers for you?

Your patch is not useful because it masks the problem but doesn't fix it.
2013-03-06 15:25:44 roderick (dot) braun (at) ph-freiburg (dot) de Comment #1
Type ⇒ Bug
State ⇒ Unconfirmed
Priority ⇒ 1. Low
Summary ⇒ Export of mailfolders fails with cyrus imap
Queue ⇒ Horde Groupware Webmail Edition
Milestone ⇒
Patch ⇒ Yes
New Attachment: imap-update-mapping-fix.patch Download
Reply to this comment
Export of mailfolders is not working. In the log you find many PHP 
errors depending  of the number of mails the exported mbox folder 
contains.
The problem is in the method "update" in .../Imap/Client/Ids/Map.php 
which is not checking whether the mapping array contains only valid 
values (I guess they should always be int?).

However, assuring that all keys and values in the array are of type 
int is solving the problem (for us).

We use the cyrus-imapd 2.4.17

PHP error msg:
---- snip ---
Mar  6 15:22:15 mailweb HORDE:  1. Horde_Registry->callAppMethod() 
/srv/www/horde/services/download/index.php:33  2. 
call_user_func_array() /srv/www/horde/pear/php/Horde/Registry.php:1141 
  3. IMP_Application->download()  4. IMP_Ui_Folder->generateMbox() 
/srv/www/horde/imp/lib/Application.php:521  5. IMP_Imap->fetch() 
/srv/www/horde/imp/lib/Ui/Folder.php:102  6. IMP_Imap->__call() 
/srv/www/horde/imp/lib/Ui/Folder.php:102  7. call_user_func_array() 
/srv/www/horde/imp/lib/Imap.php:385  8. 
Horde_Imap_Client_Base->fetch()  9. array_flip() 
/srv/www/horde/pear/php/Horde/Imap/Client/Base.php:2552 10. 
Horde_ErrorHandler::errorHandler()
Mar  6 15:22:15 mailweb HORDE: [imp] PHP ERROR: array_flip(): Can only 
flip STRING and INTEGER values! [pid 3772 on line 2552 of 
"/srv/www/horde/pear/php/Horde/Imap/Client/Base.php"]
--- snap ---

Saved Queries