105266 - cannot delete or move a certain mail message

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect, P1)

Description

[pete]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:0.9.5+) Gecko/20011015
BuildID:    2001101503

in my 'inbox' i have received a piece of SPAM junkmail. strange thing is, i
cannot delete or move the mail. it is just stuck in my mailbox. all other
messages can be received/moved/deleted as normal, there is just one message
stuck in my inbox. the message has been there for about 2 weeks, so this bug has
been in the nightlies at least since then.

this is on a POP account. i've tried many things, but the message cannot be
moved or deleted. a message in the status bar "Moving message 1 of 1 to trash"
appears, and a progress bar appears about 20% completed, but it gets stuck and
the message doesn't go away.

since i don't have my POP settings immediately delete the mail, i have this
message actually stuck on 2 computers. one winNT and one debian (running 0.9.5
right now)

the INBOX for this account is fairly small, just 4-5 messages, should i send the
"inbox" and "inbox.msf" as an attachment?

the only "special" thing about the message is it is encoded in some "russian"
looking font. and might have some sort of javascript included?



Reproducible: Always

Comment 1

[pete]

Attachment: the undeleteable email (spam)

Comment 2

[pete]

after 2 weeks, i've just received a second piece of SPAM. the exact same letter
from whomever gave me the first one. exact same problem with this mail, i cannot
delete it or move it.

i saved the letter an a .EML, and have attached it.

Comment 3

[Sheela Ravindran]

Confirming this using build: 2001-10-31-06
Real bad message.  But when I saved the message as link from the browser to my 
desktop and copied to my local folder. I was not able to see any header info 
that we display in the envelope area in the 3 pane window.
I was not able to delete this message at all. 

Comment 4

[Sheela Ravindran]

reassign

Comment 5

[Mike Young]

I also see this message at my work with an email with several attachments. It
takes up a huge amount of space and cannot be deleted. I can delete it if I open
the inbox file in a text editor, but otherwise it just won't go away.... I'll
attach the file soon

Comment 6

[David :Bienvenu]

the message probably doesn't have an x-mozilla-status line, for some reason. We
need that to remember that we've deleted the message. Also, be aware that you
have to compact a folder for the message to actually be deleted from disk.

Comment 7

[Mike Young]

OK, I finally got into work today, and was able to get the copy of the inbox
that was causing the problem. (the inbox file was 95MB with only 25 messages!)

The message were were dealing with had 4 attachments and did have an
X-Mozilla-Status line. Unfortunately, I have been informed that I cannot upload
the inbox file because it contains confidential information.

I was able to get Mozilla (Netscape) to delete the message by emptying the
trash, compacting folders (which shrunk the inbox from 95MB to 300K) and then
deleting the message normally. 

I'm wondering if this might have something to do with mozilla getting confused
about the location of the message in the inbox file with all of that cruft in there.

This computer also has Netscape set to leave messages on the server until deleted. 

Is it true that we do not delete the message until we empty the trash, and that
we actually leave it on the server otherwise? 

We really should do some sort of clean-up to prevent people from getting 95MB
inboxes.

Comment 8

[Sheela Ravindran]

Reporter,
You can check a pref to auto compact folders when you log into your mail.
Go to preferences|offline and disk space then check the pref " Compact folders 
when it will save over[n] kb. So every time you have log into mail there will a 
prompt which will ask you if you want to compact. This will save disk space for 
you.  

Leave messages on the server, delete on the server when deleted locally should 
clean up the inbox and on the server when it is followed by a Get message.  So 
if you delete a message and want this message to be deleted on the server then 
click on Get message in that session. The message gets removed from the server 
too. 

Comment 9

[Navin Gupta]

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

Comment 10

[Navin Gupta]

The problem is that this message have very long lines that exceed the limit
as noted in RFC2822. Now do we want to make our app 6.x more robust and 
handle this case because 4x also handles this case ?



2.1.1 Line Length Limits

     There are two limits that this standard places on the number of characters
in a line. Each line of characters MUST be no more than 998  characters, and
SHOULD be no more than 78 characters, excluding the CRLF. 

     The 998 character limit is due to limitations in many implementations which
send, receive, or store Internet Message Format messages that simply cannot
handle more than 998 characters on a line. Receiving implementations would do
well to handle an arbitrarily large number of characters in a line for
robustness sake. However, there are so many implementations which (in compliance
with the transport requirements of [RFC2821]) do not accept messages containing
more than 1000 character including the CR and LF per line, it is important for
implementations not to create such messages. 

     The more conservative 78 character recommendation is to accommodate the
many implementations of user interfaces that display these messages which may
truncate, or disastrously wrap, the display of more than 78 characters per line,
in spite of the fact that such implementations are non-conformant to the intent
of this specification (and that of [RFC2821] if they actually cause information
to be lost). Again, even though this limitation is put on messages, it is
encumbant upon implementations which display messages 

Comment 11

[David :Bienvenu]

It would be highly desirable to handle this like 4.x - how hard do you think it
would be?

Comment 12

[Navin Gupta]

I am working on a fix, we will dynamically allocate buffer size.

Comment 13

[Navin Gupta]

Attachment: proposed fix

The fix is to realloc the buffer that is used for copying msgs line by line.
This needs to be done for both imap and local and I have tested single/multiple

msgs copied between news, local, and imap that executes this code.

for imap I have a found a different problem after this msg ( 54089: the
undeleteable email (spam)) is uploaded to the server. displaying the msg causes

a minor hang (throbber keeps going and msg is not loaded in msg-pane) but
I know copy is correct because 4x displays the msg correctly, also copying to
temp file before uploading to the server looks good.

Comment 14

[Navin Gupta]

david, can you review

Comment 15

[Navin Gupta]

logged bug 122894 for the imap message loading doesn't finish.

Comment 16

[David :Bienvenu]

are we always reallocing, no matter what? If so, I think it would be better to
only realloc if we need more space, which I guess would require keeping track of
the length of the allocated block. 

You can use PR_FREE instead of PR_FREEIF in the destructors for both copy
states, since PR_FREE already checks for null (as I learned from you :-) ), and
you don't need to null out the pointer if we're already in the destructor.

Note that PR_REALLOC does NOT null out the allocated memory. Previously, we were
using PR_CALLOC, which does null out memory. Is that an issue? Does the code
assume that the allocated block is nulled out?

Comment 17

[Navin Gupta]

PR_REALLOC is the way to go because we ay  have leftOver from previous
OnDataAvailable As you may know PR_REALLOC just increases the size of the buffer
while not touching the contents of the buffer. this is what we want here.

Regarding PR_FREE we have not been following that quite religiously when I 
was fixing leaks but I can change it. 

Comment 18

[David :Bienvenu]

realloc might also shrink an existing buffer, if I understand correctly. So,
everytime through this code, you might end up allocating a new buffer, and
copying the existing data into the new buffer. Perhaps, realloc will always
return you the same buffer if you ask for a smaller buffer, but I see uses in
our code that assume that realloc'ing a smaller size will give you a new,
smaller buffer. 

Comment 19

[Navin Gupta]

you are right it may cause to shrink in some cases but our content would be
left untouched because we have a lower bound of m_leftOver, we are
always allocating aLength + m_copyState->m_leftOver + 1

Comment 20

[David :Bienvenu]

I'm probably leaving lots of stuff out here. PR_Realloc will increase the size
of the buffer, but it very likely will have to allocate a new buffer, copy the
data over to it, and return the new buffer. We should not assume that it will
always be able to shrink or grow the buffer without allocating a new buffer and
copying the data. If CopyData gets called with different amounts each time,
worst case, we'll end up allocating a new buffer each time and copying the full
contents of the buffer (most of which we've already dealt with and don't need
copied at all). That's why I'm suggesting that we only realloc when we truly
need to grow the buffer.

Comment 21

[Navin Gupta]

As per linux manual if buffer size is 0 realloc does a malloc
buffer = realloc(buffer, len);

Now most of the time it will be above case because buffer ends w/ line break
so we will hit this code

 if (start >=
         &mCopyState->m_dataBuffer[mCopyState->m_leftOver])
    {
      PR_Free(mCopyState->m_dataBuffer);
      mCopyState->m_leftOver = 0;
      break;
    }

Now for the other time when we have leftOver, then we move the leftOver
bytes to start of buffer here

  if (start && !end)
    {
      mCopyState->m_leftOver -= (start - mCopyState->m_dataBuffer);
      memcpy (mCopyState->m_dataBuffer, start,
              mCopyState->m_leftOver+1);
    }
and mCopyState->m_dataBuffer[m_leftOver] = 0
so we are copying only leftOver bytes, the next time we do realloc, right. 

Comment 22

[David :Bienvenu]

I must be misreading the code - from what I can tell, we will call realloc for
every block of data we get to in CopyData, and we will copy all the data in the
buffer, if realloc needs to allocate a new block. For example, if I select 5
local messages and press delete, CopyData will get called 5 times, with
different lengths each time, with the same copy state object. In the worst case,
we would allocate 5 blocks of memory, and copy the old block of memory into the
new buffer (which is useless because we don't need it at all). I tried this in
the debugger, and that's what seemed to happen.

IF I'm reading the code correctly (a big if), it would seem to me that the
safest change would be to just change the code that gets run in the case where
we do get the extra long line and need the extra big buffer - in that case, we
could just realloc the buffer and get on with things.

Comment 23

[Navin Gupta]

If we delete 5 local msgs BeginCopy will be called 5 times and we are 
freeing buffer there. Also 99% of the times we will hit this code
in CopyData when we are finished with copyData because all the 
buffer have linebreaks at the end. so we are safe and there is
no extra copying from buffer between messages

if (start >=
         &mCopyState->m_dataBuffer[mCopyState->m_leftOver])
    {
      PR_Free(mCopyState->m_dataBuffer);
      mCopyState->m_leftOver = 0;
      break;
    }

Comment 24

[David :Bienvenu]

Before your change (correct me if I'm wrong), we would only allocate the 16K
buffer once in the "delete 5 messages" scenario, whereas, with your change, we
would perform at least 5 different allocations and frees. With your change, if
we copy a large message (> 16K), we would allocate and free a 16K block for
every 16K chunk of the message (assuming CopyData gets called with a 16K block
every time). Without your change, we would only allocate the 16K block once, and
reuse it.

Comment 25

[Navin Gupta]

Attachment: proposed fix

bienvenu is right - realloc again and again will decrease performance and also
that realloc will most likely do a block copy instead of looking for null. 

This fix prevents realloc unless needed and between multiple deletes we don't
realloc. bienvenu, can yor review ?

Comment 26

[David :Bienvenu]

Comment on attachment 67756 [details] [diff] [review]
proposed fix

r=bienvenu, looks good.

Comment 27

[David :Bienvenu]

just to be clear, realloc will definitely not look for a null byte in the
allocated block. That would be wrong - the memory allocation routines like
malloc, free, realloc treat the memory as blocks of arbitrary binary memory, not
strings.

Comment 28

[Navin Gupta]

fixed. 

Comment 29

[Navin Gupta]

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

Comment 30

[laurel]

OK using mar28 commercial trunk build: win98, linux rh6.2, mac OS 10.1
Tested deleting this message from local folder.