226669 - Thunderbird tries to download nonexisting messages with POP3

Status: RESOLVED FIXED

(MailNews Core :: Networking: POP, defect)

Description

[Tim Kosse]

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

Comment 1

[Christian Eyrich]

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?

Comment 2

[David :Bienvenu]

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.

Comment 3

[Christian Eyrich]

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.

Comment 4

[Christian Eyrich]

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

Comment 5

[Christian Eyrich]

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

[David :Bienvenu]

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.

Comment 7

[Christian Eyrich]

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.

Comment 8

[Christian Eyrich]

Attachment: 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.

Comment 9

[Christian Eyrich]

Attachment: 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).

Comment 10

[David :Bienvenu]

Comment on attachment 143785 [details] [diff] [review]
patch v2

looks good to me

Comment 11

[David :Bienvenu]

Comment on attachment 143785 [details] [diff] [review]
patch v2

requesting mscott review.

Comment 12

[David :Bienvenu]

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.

Comment 13

[:aceman]

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).

Comment 14

[Christian Eyrich]

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: <[e-mail]>
we'll extract "Message-ID:" as UIDL ...
Reactivating the aboe line would fix this problem.

Comment 15

[David :Bienvenu]

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.

Comment 16

[Christian Eyrich]

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.

Comment 17

[Christian Eyrich]

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

Comment 18

[David :Bienvenu]

fix checked in, thx, Christian!

Comment 19

[Christian Eyrich]

Reopen to change assignee.

Comment 20

[Daniel Veditz [:dveditz]]

Blocking 1.7.3

Comment 21

[:aceman]

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

[David :Bienvenu]

I don't think this was checked into 1.7...

Comment 23

[Daniel Veditz [:dveditz]]

This has not landed on the 1.7 branch; it should.

Comment 24

[Daniel Veditz [:dveditz]]

Comment on attachment 143785 [details] [diff] [review]
patch v2

a=dveditz for drivers

Comment 25

[Daniel Veditz [:dveditz]]

Fix checked in to 1.7 and 1.7.2 branches