523811 - crash navigating news articles @ nsNNTPProtocol::DisplayArticle

Status: RESOLVED FIXED

(MailNews Core :: Networking: NNTP, defect)

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

Comment 7

[Wayne Mery (:wsmwk)]

Aceman, thainks for all the analysis. So this is an easy win? Are there many callers that need to be changed when fixing nsMsgLineBuffer?

I checked and nothing changed yet in https://dxr.mozilla.org/comm-central/rev/2a29ee0adb310b54a6a2df72034953fed8f2b043/comm/mailnews/base/util/nsMsgLineBuffer.cpp#313

Crashing line is now https://hg.mozilla.org/comm-central/annotate/tip/mailnews/news/src/nsNNTPProtocol.cpp#l2023

Comment 8

[Wayne Mery (:wsmwk)]

bp-fe292625-2221-413f-b9ce-11a3e0190411
bp-065d3e7b-2d1f-4511-b5d0-dc9ed0190405
bp-b8ab70cc-5984-405d-aa05-cf8620190320

FWIW, other news crashes - probably none related

• open bugs https://mzl.la/2JD65Rt
• fixed, dupe and open from past 2 years https://mzl.la/2JD2vqo

Comment 9

[:aceman]

Attachment: 523811.patch

I have looked at all the callers of ReadNextLine and it seems to me ALL of them check either rv or !line so for those nothing changes if we change return value of aNumBytesInLine.
Only the single caller in question inside nsNNTPProtocol::DisplayArticle (at https://hg.mozilla.org/comm-central/annotate/tip/mailnews/news/src/nsNNTPProtocol.cpp#l1989) does not check either one, but checks the number of bytes. So this patch changes it to return 0.
Let's see what happens. It seems to me in case of 0 we hit the line:
mDisplayOutputStream->Write(line, line_length, &count);

I don't know what happens when this runs with line==nullptr and line_length==0. But at least we do not access line[0] in the check above.

Maybe we get a crash at this place, but then we know we are fixing the right cause :) When that happens we may need to add proper check for NS_FAILED(rv) or !line, but then I don't know how to return, is it protocol specific logic :) If jcranmer does not help, we will have to guess :)

So, let's try the simple version for now.
Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4fd248a7a5c857644a746d68a0b9fa027b357c5b

Comment 10

[Jorg K (CEST = GMT+2)]

Comment on attachment 9064555 [details] [diff] [review]
523811.patch

Sorry, but I don't like this. Not returning MAXINT/-1 on an unsigned int is a step forward.

However, you will return line==nullptr and that will get you into the next trouble. I see two options:

Either after
https://searchfox.org/comm-central/rev/a6f313573be4254edafe723456289680f12b0101/mailnews/news/src/nsNNTPProtocol.cpp#2001
do: `if (!line) return NS_OK;`

Second option:
Change the `esle` here
https://searchfox.org/comm-central/rev/a6f313573be4254edafe723456289680f12b0101/mailnews/news/src/nsNNTPProtocol.cpp#2026
to an `else if (line)`. Surely passing null to `Write` will/may crash.

Comment 11

[:aceman]

Attachment: 523811.patch v1.1

OK, I'm fine with going with plan B directly. I'm just not sure about returning NS_OK, but we will see.

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 9064590 [details] [diff] [review]
523811.patch v1.1

Well, before we crashed, now the article won't be displayed correctly, I guess we can live with that.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7d83fbb674d6
do not return -1 in a uint32_t argument in nsMsgLineStreamBuffer::ReadNextLine() to prevent an overflow. r=jorgk

Comment 14

[Jorg K (CEST = GMT+2)]

Fingers crossed. This is an uplift candidate.

Comment 15

[Wayne Mery (:wsmwk)]

Note - as can be seen in the graph this has crazy low crash volume, you could almost say subzero :) I'd just let it ride the train.

Comment 16

[Jorg K (CEST = GMT+2)]

TB 60.8 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/48a03f2b366309f62cc5184f3a18a6960520b526

Grrr, I messed up the rebasing: aNumBytesInLine = -0;, doesn't matter.