523811 - crash navigating news articles [@ nsNNTPProtocol::DisplayArticle(nsIInputStream*, unsigned int)]

Status: NEW

Description

[Wayne Mery (:wsmwk)]

crash navigating news articles [@ nsNNTPProtocol::DisplayArticle(nsIInputStream*, unsigned int)]

#7 crash for current trunk (someone is on a run). but not in 3.0b4 top 100. not a new crash


bp-77ac7e0c-9ac5-494b-bf11-4e4a12090709
Just clicked on a newsgroup and pressed space to go to the next unread message
0	thunderbird.exe	nsNNTPProtocol::DisplayArticle	 mailnews/news/src/nsNNTPProtocol.cpp:2525
1	thunderbird.exe	nsNNTPProtocol::ReadArticle	mailnews/news/src/nsNNTPProtocol.cpp:2548
2	thunderbird.exe	nsNNTPProtocol::ProcessProtocolState	mailnews/news/src/nsNNTPProtocol.cpp:5122 


Mac stack is different
bp-4c432ab8-df4d-4d58-afd9-182392090814
0	thunderbird-bin	nsNNTPProtocol::DisplayArticle	 mailnews/news/src/nsNNTPProtocol.cpp:2525
1	thunderbird-bin	nsNNTPProtocol::ReadArticle	mailnews/news/src/nsNNTPProtocol.cpp:2548
2	thunderbird-bin	nsNNTPProtocol::ProcessProtocolState	mailnews/news/src/nsNNTPProtocol.cpp:5122
3	thunderbird-bin	nsMsgProtocol::OnDataAvailable	mailnews/base/util/nsMsgProtocol.cpp:351
4	thunderbird-bin	nsInputStreamPump::OnStateTransfer	netwerk/base/src/nsInputStreamPump.cpp:508
5	thunderbird-bin	nsInputStreamPump::OnInputStreamReady	netwerk/base/src/nsInputStreamPump.cpp:398
6	libxpcom_core.dylib	nsInputStreamReadyEvent::Run	xpcom/io/nsStreamUtils.cpp:111
7	libxpcom_core.dylib	nsThread::ProcessNextEvent	xpcom/threads/nsThread.cpp:510

Comment 1

[Joshua Cranmer [:jcranmer]]

Hmm...

if (line_length > 1 && line[0] == '.' && line[1] == '.') is the crashing line, so that means that line was returned null.

Following backwards, the line stream buffer would have returned null line but not wait for more data. line_length would also have to be > 1.

Reading nsMsgLineBuffer.cpp:
char * nsMsgLineStreamBuffer::ReadNextLine(nsIInputStream * aInputStream, PRUint32 &aNumBytesInLine, PRBool &aPauseForMoreData, nsresult *prv, PRBool addLineTerminator)

Note the arguments.

    if (NS_FAILED(rv))
    {
      if (prv)
        *prv = rv;
      aNumBytesInLine = -1;
      return nsnull;
    }

Sigh. There's the cause of the crash. Now fixing that would be interesting...

Comment 2

[abittner]

was trying to repro some other nntp relate bug
https://bugzilla.mozilla.org/show_bug.cgi?id=531794#c19

but landed here with todays Thunderbird 3.1a1pre build 20100105033430

crash signature and comment is at:
http://crash-stats.mozilla.com/report/index/bp-198a8c58-9321-4615-a0dc-554832100105

Comment 3

[Joshua Cranmer [:jcranmer]]

(In reply to comment #2)
> was trying to repro some other nntp relate bug
> https://bugzilla.mozilla.org/show_bug.cgi?id=531794#c19
> 
> but landed here with todays Thunderbird 3.1a1pre build 20100105033430
> 
> crash signature and comment is at:
> http://crash-stats.mozilla.com/report/index/bp-198a8c58-9321-4615-a0dc-554832100105

This isn't the same crash: the lines and causes are definitely different.

Comment 4

[Wayne Mery (:wsmwk)]

(In reply to Joshua Cranmer [:jcranmer] from comment #1)
> Sigh. There's the cause of the crash. Now fixing that would be interesting...

#1 news crash (but not a topcrash)

Comment 5

[Wayne Mery (:wsmwk)]

only about a dozen crashes per week
bp-ff0268d3-4194-4e10-9053-cb31d2130710

Comment 6

[:aceman]

(In reply to Joshua Cranmer [:jcranmer] from comment #1)
> Hmm...
> 
> if (line_length > 1 && line[0] == '.' && line[1] == '.') is the crashing
> line, so that means that line was returned null.
> 
> Following backwards, the line stream buffer would have returned null line
> but not wait for more data. line_length would also have to be > 1.
> 
> Reading nsMsgLineBuffer.cpp:
> char * nsMsgLineStreamBuffer::ReadNextLine(nsIInputStream * aInputStream,
> PRUint32 &aNumBytesInLine, PRBool &aPauseForMoreData, nsresult *prv, PRBool
> addLineTerminator)
> 
> Note the arguments.
> 
>     if (NS_FAILED(rv))
>     {
>       if (prv)
>         *prv = rv;
>       aNumBytesInLine = -1;
>       return nsnull;
>     }
> 
> Sigh. There's the cause of the crash. Now fixing that would be interesting...

So basically the problem is that aNumBytesInLine is uint32 and we assign -1 to it that wraps to become a big number and all tests checking length > 1 pass now.
So we need to return some better number (e.g. 0) and also check 'rv' and 'line!=null' at the callers (like after http://hg.mozilla.org/releases/comm-esr17/annotate/075d9a47130b/mailnews/news/src/nsNNTPProtocol.cpp#l2122).