Thunderbird tries to download nonexisting messages with POP3

RESOLVED FIXED

Status

MailNews Core
Networking: POP
RESOLVED FIXED
14 years ago
8 years ago

People

(Reporter: Tim Kosse, Assigned: Christian Eyrich)

Tracking

({fixed-aviary1.0, fixed1.7.3})

Trunk
fixed-aviary1.0, fixed1.7.3
Dependency tree / graph
Bug Flags:
blocking1.7.5 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:fix])

Attachments

(1 attachment, 1 obsolete attachment)

8.53 KB, patch
Scott MacGregor
: review+
Bienvenu
: superreview+
dveditz
: approval1.7.5+
Details | Diff | Splinter Review
(Reporter)

Description

14 years ago
User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; .NET CLR 1.1.4322)
Build Identifier: Mozilla Tunderbird 0.4a (20031119)

I'm using a small pop3 proxy which scans my incoming mail headers and deletes 
spam directly on the server. 

However if I try to get my mail using Thunderbird over my proxy, it always 
fails, I get an "-ERR message is deleted" response from the server. 

Thunderbird correctly sends LIST and UIDL to the server. My proxy did already 
delete some messages, so that they aren't included in the response to LIST/UIDL 
anymore. 

Example (message with index 2 has been deleted): 

UIDL 
+OK mailbox has 2 messages (23612 octets) 
1 4dc74a92bfa4d7s8 
3 613d71dfdc905ee0 
 
The next thing Thunderbird does is to retrieve the mails with the UIDs it 
doesn't know yet. The problem is that Thunderbird also sends a RETR 2 command, 
which fails. 
I could think of the following reasons for this bug: 
- Thunderbird does not correctly parse the output of LIST/UIDL 
- the internal UID list does not get initialized properly (no UID for mail with 
index 2 received -> 0 or random data as UID) 
- Thunderbird tries to download messages with 0 UIDs (in case the internal UID 
list gets zero-initialized) 

I've tested the Win32 build of Mozilla Tunderbird 0.4a (20031119) 

Reproducible: Always

Steps to Reproduce:




I've been redirected to BugZilla, the original report can be found here: 
http://forums.mozillazine.org/viewtopic.php?p=274792#274792
(Assignee)

Comment 1

14 years ago
Oh, that's a problem, yes.
We imply that we're the only one doing something with the mails in the current
session. Therefore we take line one as message one, line two as message two a.s.o.
But that's not true if a proxy deletes messages in the same session. RFC clearly
says "Note that messages marked as deleted are not listed."

I guess we have to include the message number in the Pop3MsgInfo struct and not
take index x for message x. That would be easy, but accessing the infos for a
message would be complicated (loop with m_pop3ConData->msg_info[i].msgnum ==
msg_num instead of msg_info[msg_num]).

David, any other idea?
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All

Comment 2

14 years ago
If that turns out to be painful, we could just leave (initialized) holes in the
msginfo struct and ignore them when we iterate over it. I'd have to look at the
code more closely.
Assignee: sspitzer → bienvenu
(Assignee)

Comment 3

14 years ago
The problem with these holes is, that with say highest message number 20 we'd
need 20 entries in m_pop3ConData->msg_info. But we don't know about the number
needed before the complete listing.
Currently we use number_of_messages filled by the STAT answer in GetStat() - but
"Note that messages marked as deleted are not counted in either total."

So we'd either could always allocate kLargeNumberOfMessages entries. Or issue
the LIST command to just look at the last message number, allocate it and then
issue LIST again to use it like now. Both not satisfying.
(Assignee)

Comment 4

13 years ago
*** Bug 230294 has been marked as a duplicate of this bug. ***

Updated

13 years ago
Blocks: 237131
(Assignee)

Comment 5

13 years ago
David, I think it's better you're doing this. I don't get what you mean with
"level of indirection" and have no own idea to fix the problem from my comment #1.

Comment 6

13 years ago
by level of indirection, I just meant what you described in commment 1, but
that's probably not a good way of describing it. I'll look and see how hard it
will be. Someone to help test this is going to be important, since I don't have
access to a server that has this behaviour.
(Assignee)

Comment 7

13 years ago
Ah, ok, so you meant the "looping-thing". Well, it should be quite easy - and
quite ugly. But as I wrote, I don't know better way.

I can test it with my Perl script that I always use - send it to you if you like.
(Assignee)

Comment 8

13 years ago
Created attachment 143729 [details] [diff] [review]
example patch

This is the patch like I'd do it.

Instead
  for(i = 0; m_pop3ConData->msg_info[i].msgnum != msg_num && i <=
m_pop3ConData->number_of_messages; i++)
    ;
  m_pop3ConData->msg_info[i].uidl = PL_strdup(uidl);
we could simply do
  m_pop3ConData->msg_info[m_listpos - 1].uidl = PL_strdup(uidl);

Required for this is that we can be sure the order and number in the UIDL and
XTND responses is the same as in the LIST response. And I don't if we really
can.
(Assignee)

Comment 9

13 years ago
Created attachment 143785 [details] [diff] [review]
patch v2

Ups, in the first patch I forgot the changed header file.

Two additional changes are in this patch:
1. If LIST, XTND or UIDL lists fewer entries than STAT reported
m_pop3ConData->number_of_messages is limited to the new maximum.
2. m_pop3ConData->msg_info[m_listpos - 1].msgnum == msg_num is tried first and
taken if true. If not, the loop searching for the right index to msg_num is
entered. I think that's best of both worlds (fast & paranoid).
(Assignee)

Updated

13 years ago
Attachment #143729 - Attachment is obsolete: true

Comment 10

13 years ago
Comment on attachment 143785 [details] [diff] [review]
patch v2

looks good to me
Attachment #143785 - Flags: superreview+

Comment 11

13 years ago
Comment on attachment 143785 [details] [diff] [review]
patch v2

requesting mscott review.
Attachment #143785 - Flags: review?(mscott)

Comment 12

13 years ago
re 1.7b and 1.7, I'd like to get it in but I'm not sure how risky it is. It's
supposed to fix a couple bugs in our handling of pop3 servers doing the wrong
thing - I'm not sure how many other users are experiencing this. I can try
running with this patch.

Updated

13 years ago
Attachment #143785 - Flags: review?(mscott) → review+

Comment 13

13 years ago
I would test it too, if it would be a matter of replacing some .dll, otherwise I
have to wait for 1.7... And the server may get fixed by that time (because it
does these things only for several days).
(Assignee)

Comment 14

13 years ago
Maybe that's another bug, but since it's about getting the right UIDL and could
be changed in the same patch.

Our XTND XLST response parser is broken. Someone commented out
  char *eatMessageIdToken = nsCRT::strtok(newStr, " ", &newStr);
in GetXtndXlstMsgid() sometime in the past.
So if we get a response 
  1 Message-ID: <3117E4DC.2699@netscape.com>
we'll extract "Message-ID:" as UIDL ...
Reactivating the aboe line would fix this problem.

Comment 15

13 years ago
Christian, that line was commented out in the very first version of Mozilla,
which means it was commented out in the pre-mozilla source code. Unfortunately,
that means I can't tell why it was commented out since that history is lost. I'm
not saying that means uncommenting it out is wrong; just that it's been this way
for 5 years at least.
(Assignee)

Comment 16

13 years ago
Ah, ok. I know that XTND XLST is unlikely to be supported by a server and maybe
the feature has never been used. But I think if we offer a command, it should work.
(Assignee)

Comment 17

13 years ago
David, did you evaluate the risk of this patch yet? If ok, we should ask for
approval.

Comment 18

13 years ago
fix checked in, thx, Christian!
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Comment 19

13 years ago
Reopen to change assignee.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

13 years ago
Assignee: bienvenu → ch.ey
Status: REOPENED → NEW
(Assignee)

Updated

13 years ago
Status: NEW → RESOLVED
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED

Updated

13 years ago
Whiteboard: fixed-aviary1.0
Blocking 1.7.3
Flags: blocking1.7.3+
Whiteboard: fixed-aviary1.0 → [sg:fix] fixed-aviary1.0, needed1.7

Comment 21

13 years ago
Is this in 1.7.0? I have tried it and I see no difference to 1.5. I was
currently hit the bug.

Comment 22

13 years ago
I don't think this was checked into 1.7...
This has not landed on the 1.7 branch; it should.
Keywords: fixed-aviary1.0
Whiteboard: [sg:fix] fixed-aviary1.0, needed1.7 → [sg:fix] needed1.7
Comment on attachment 143785 [details] [diff] [review]
patch v2

a=dveditz for drivers
Attachment #143785 - Flags: approval1.7.3+
Blocks: 245066
Fix checked in to 1.7 and 1.7.2 branches
Keywords: fixed1.7.3
Whiteboard: [sg:fix] needed1.7 → [sg:fix] fixed1.7.2+
Whiteboard: [sg:fix] fixed1.7.2+ → [sg:fix] fixed1.7.3
Product: MailNews → Core
Keywords: fixed1.7.5 → fixed1.7.3
Whiteboard: [sg:fix] fixed1.7.3 → [sg:fix]
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.