87312 - long news article line length crashes NNTPProtocol::ReadArticle

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Backend, defect)

Description

[Bruce Robson]

I've seen this crash with the windows talkback build 0.9.1 that I
downloaded and also with a build I made from the 0.9.1 sources.


Mozilla is crashing when downloading articles from news group
microsoft.public.vb.bugs. I'm reading the news group from news server 
msnews.microsoft.com 


What happens is
1)  nsNNTPProtocol::ReadArticle calls nsMsgLineStreamBuffer::ReadNextLine
    I stepped through this function and found that it 
    a)  finds 356 bytes available in the stream and 161 free in the buffer
    b)  grows the buffer after which m_databuffer is 0x05873fc0
    c)  finds End of line at 0x0587607c (8380 byte line)
    d)  buffer newLine allocated at 0x05876188 and returned

2)  we return to nsNNTPProtocol::ReadArticle and line is set to 0x05876188
    (n.b. it points at 8380 + 1 bytes)

3)  nsNNTPProtocol::ReadArticle continues until it reaches the line
        PL_strcpy (outputBuffer, line);
    outputBuffer is "only" 8192 bytes so we corrupt the stack (including the
    local variable line).

4)  With a debug build I get an assert from the microsoft free() function inside
    the PR_FREEIF(line) call at the end of nsNNTPProtocol::ReadArticle.

    With a non-debug build we return to a random address and the system crashes.


I see from the comments in nsNNTPProtocol.cpp that the OUTPUT_BUFFER_SIZE 
started at 2000, was increased to 4k and was increased again to 8k. Increasing
the size again does not seem sensible as whatever size is choosen will
eventually be exceeded by a news article.

nsNNTPProtocol::ReadArticle is only copying the line into a local buffer so that
it can add a line terminator. we need to
a)  change the algorithem so we don't need the line + line terminator.
    I expect m_tempArticleStream->Write could be called twice (once for the line
    and once for the line terminator). What about ParseHeaderForCancel? Does
    this really need a line terminator?

    This gets rid of the copy and the scan of the string to find the end.
or
b)  change nsMsgLineStreamBuffer::ReadNextLine so that the buffer it allocates
    either already has the line terminator or has room for adding the line
    terminator.

    This gets rid of the copy but leaves the scan of the string to find the end.
or
c)  dynamically allocate the buffer for the line + line terminator.

    Doing this by a PR_Realloc() on line leaves the scan of the string to find
    the end but may (depending on what the realloc() does) get rid of the copy.

Comment 1

[Bruce Robson]

I've run the talkback program to look at the incidents I've reported.
I've reported two (TB31577878X and TB31918594Y) whilst running the
prebuilt 0.9.1. I think they were both caused by this.

Comment 2

[Bruce Robson]

Attachment: Suggested fix.

Comment 3

[Bruce Robson]

The suggested fix implements fix route (a).
n.b. The diff is against the version (1.268 ?) of nsNNTPProtocol.cpp in
    Mozilla 0.9.1 not the latest version.

I built Mozilla with this change and have been able to download the
newsgroup.

Comment 4

[Bruce Robson]

I've thought about it some more. What's really happening is that
we are downloading articles from the server for offline reading.

Function nsNNTPProtocol::ReadArticle is complicated because it is not 
implementing just this task. It appears to also have something to do with
cancelling messages. It also has m_tempArticleStream which I don't understand
(this wasn't used when I steped through the function. m_newsFolder was set
and was the data destination). It might be better to have seperate functions  
for the seperate purposes and make the caller determine which to call.
Alternatively test near the top of the function and have separate paths for
the different tasks.

Also nsIOutputStream has a WriteFrom method that takes data directly from an 
nsIInputStream. Wouldn't it be better to just identify the length of the
next line and use this method instead of using the line buffer.

This is all more work than I'm going to do. With my first patch it's
inefficent but seems to work.

Comment 5

[David :Bienvenu]

sorry, I've been on vacation. Could you attach a diff -uw so I can see what you
actually changed? Thanks!

Comment 6

[Bruce Robson]

Attachment: diff -u of the sugested patch

Comment 7

[David :Bienvenu]

I tried out this patch; it seems to work for me and looks ok. Seth, can I get an
sr=? I did a quick check and cancel still works. Re some of Bruce's questions,
we need to use the line buffer so that we can individual lines out of the
stream; otherwise, we could have tried to use WriteFrom (though I haven't had
any luck with that method, actually).

Comment 8

[(not reading, please use [e-mail] instead)]

bruce, which article in that group is crashing for you?

Comment 9

[David :Bienvenu]

I should say that I reproduced the crash before applying Bruce's patch and
verified that I don't crash after applying the patch.

Comment 10

[(not reading, please use [e-mail] instead)]

ok, I can reproduce the crash.

here is the message that is horking us:

news://msnews.microsoft.com/%23ptYHBmPAHA.287%40cppssbbsa05

Comment 11

[(not reading, please use [e-mail] instead)]

based on bienvenu's comments, sr=sspitzer

thanks for the fix, bruce.

Comment 12

[David :Bienvenu]

fix checked in. Thanks again, Bruce.

Comment 13

[Bruce Robson]

I've used bonsai to look at the change that was checked into the tree.
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/mailnews/news/src&command=DIFF_FRAMESET&file=nsNNTPProtocol.cpp&rev2=1.275&rev1=1.274

This check makes the change I suggested but also deletes 2 other lines
        // now mark the message as read                                         
                                                                               
                                                                    
MarkCurrentMsgRead();                                                   

                                                                               
                                                             Were these lines
deleted by accident or was this done deliberately?
The comment on the check-in only refers to this bug.
  

Comment 14

[Håkan Waara]

If you believe the code was actually never checked in, reopen this bug.

Comment 15

[David :Bienvenu]

Bruce was saying his change was checked in, AND another change was checked in.
Bruce, that other change was deliberate, and was to fix a bug where when you
downloaded messages for offline use, they got marked read.

Comment 16

[Paul MacQuiddy]

Any idea how to reproduce this bug? I was told this happens in 4.x, but the 
article has expired...

Comment 17

[David :Bienvenu]

subscribe to the newsgroup the reporter specified, bring up the properties
dialog for that group,  and click Download Now. That should download all the
messages for that newsgroup. If you don't crash, then the bug is fixed.

Comment 18

[Paul MacQuiddy]

Thanks David, i'm able to reproduce it in 4.x now (I'm in 4.x sustaining)

using 4.x, i can download all the articles, but then when I click on the message 
'We will pay you $10 for all referrals' dated 10/25/00 and I crash, though the 
next time I go there it says it's expired....

Comment 19

[David :Bienvenu]

this is a 6.0 bug - I don't know if it happens in 4.x, but it's not really
important for this bug.

Comment 20

[Paul MacQuiddy]

seth had told us it happened in 4.x, so i was just following up...

thanks, again

Comment 21

[grylchan]

Commercial builds:
2001-08-15-09-trunk/ - nt 4.0
2001-08-15-14-trunk- linux 2.2, mac 9.0.4

Able to download and read messages (on/offline) in that newsgroup.
No crash occurs.

marking as verified.