1294533 - Crash in memcpy | NS_CopySegmentToBuffer rising in Firefox 49

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[[:philipp]]

[Tracking Requested - why for this release]:

This bug was filed from the Socorro interface and is 
report bp-2ce5248d-78fb-4620-b801-142f92160811.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	vcruntime140.dll 	memcpy 	f:\dd\vctools\crt\vcruntime\src\string\i386\memcpy.asm:194
1 	xul.dll 	NS_CopySegmentToBuffer(nsIInputStream*, void*, char const*, unsigned int, unsigned int, unsigned int*) 	xpcom/io/nsStreamUtils.cpp:820
2 	xul.dll 	nsPipeInputStream::ReadSegments(nsresult (*)(nsIInputStream*, void*, char const*, unsigned int, unsigned int, unsigned int*), void*, unsigned int, unsigned int*) 	xpcom/io/nsPipe3.cpp:1322
3 	xul.dll 	nsPipeInputStream::Read(char*, unsigned int, unsigned int*) 	xpcom/io/nsPipe3.cpp:1347
4 	xul.dll 	mozilla::dom::FileReader::DoReadData(unsigned __int64) 	dom/base/FileReader.cpp:336
5 	xul.dll 	mozilla::dom::FileReader::OnInputStreamReady(nsIAsyncInputStream*) 	dom/base/FileReader.cpp:614
6 	xul.dll 	nsInputStreamReadyEvent::Run() 	xpcom/io/nsStreamUtils.cpp:95
7 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1067
8 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:100
9 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc:228
10 	xul.dll 	NS_DispatchToMainThread(already_AddRefed<nsIRunnable>&&, unsigned int) 	xpcom/glue/nsThreadUtils.cpp:183

crashes with this signature seem to have become more frequent starting with firefox 49. currently it is making up 0.3% of all browser crases in 49.0b2.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Andrea, any thoughts on this one?  I'm not familiar with FileReader.

Comment 2

[Gerry Chang [:gchang]]

Track 49+ as this crash keeps showing in 49.

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late for 49 as we are heading into the RC build next Monday. 1 week till release.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

I'll look at this from a pipe angle next week.

Comment 5

[Andrea Marchesini [:baku]]

We had similar issues when the underlying file changes size. But that should be fixed by now. It could be something similar.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

Crash volume for signature 'memcpy | NS_CopySegmentToBuffer':
 - nightly (version 52): 4 crashes from 2016-09-19.
 - aurora  (version 51): 1 crash from 2016-09-19.
 - beta    (version 50): 394 crashes from 2016-09-20.
 - release (version 49): 669 crashes from 2016-09-05.
 - esr     (version 45): 0 crashes from 2016-06-01.

Crash volume on the last weeks (Week N is from 10-03 to 10-09):
            W. N-1  W. N-2
 - nightly       4       0
 - aurora        0       1
 - beta        336      58
 - release     539     130
 - esr           0       0

Affected platform: Windows

Crash rank on the last 7 days:
           Browser   Content     Plugin
 - nightly           #200
 - aurora
 - beta    #48       #41
 - release #121      #83
 - esr

Comment 8

[Marcia Knous [:marcia]]

Bug 1314046 seems to have a set of steps to reproduce this signature.

Comment 9

[Michelle Funches - QA]

Version 	52.0a1
Build ID 	20161110030211
User Agent 	Mozilla/5.0 (Windows NT 5.1; rv:52.0) Gecko/20100101 Firefox/52.0
https://crash-stats.mozilla.com/report/index/bp-17e3656e-0a4f-4f6d-82d6-3b9822161110
1) -safe-mode to start Firefox (new Profile)
2) Login to yahoo mail
3) Compose email & attach file 3GB or larger
Any idea when this might have been working? I can run mozregression again; the dates applied below still fail.
mozregression --bits 32 --good 2016-03-07 --bad 2016-09-05
 0:15.30 INFO: Running mozilla-central build for 2016-03-07
 1:24.37 ERROR: Build was expected to be good! The initial good/bad range seems
incorrect.

Comment 10

[[:philipp]]

hm, maybe it's not a real regression, but just a changing signature (maybe due to the switch to mscv2015 in 49). when i tried the STR on 45esr i got the following crash report: bp-a7950abe-0e4e-4981-819e-df7542161110 with the signature [@ msvcr120.dll@0xf20c | nsPipeInputStream::ReadSegments ]

and indeed, after the 49 release date this signature seems to be declining.
https://crash-stats.mozilla.com/signature/?product=Firefox&release_channel=release&signature=msvcr120.dll%400xf20c%20%7C%20nsPipeInputStream%3A%3AReadSegments&date=%3E%3D2016-05-10T18%3A35%3A20.000Z#graphs

Comment 11

[Eric Rahm [:erahm]]

Perhaps some help in reproducing, we're seeing this more often with the patches applied in bug 1286082, comment 12.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Eric Rahm [:erahm] from comment #11)
> Perhaps some help in reproducing, we're seeing this more often with the
> patches applied in bug 1286082, comment 12.

That stack is for an nsStorageStream instead of a pipe.  May be a different problem.

Comment 13

[[:philipp]]

the volume of this crash went up again starting from around 2016-12-10 - now many user comments also say that they experienced this crash when they tried uploading a couple of photos to a facebook album.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

This is probably some interaction between FileReader, nsPipe, and AutoIPCStream.  I still want to look at it, but not sure when I will get to it.  Andrea, do you have any ideas?

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

I feel like this is almost certainly a pipe bug.  I'd like to land some diagnostic assertions to try to narrow down what is happening here.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add some diagnostic asserts to nsPipe code to investigate null buffer crashes. r=froydnj

This patch adds some diagnostic assertions to try to catch the problem earlier and narrow down what the problem is.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Actually, I take it back.  This is *reading* from the pipe.  So the destination buffer is owned by FileReader.

Looking at the stacks I believe the Read() is happening here:

https://dxr.mozilla.org/mozilla-central/source/dom/file/FileReader.cpp#336


    if (mDataFormat != FILE_AS_ARRAYBUFFER) {
      mFileData = (char *) realloc(mFileData, mDataLen + aCount);
      NS_ENSURE_TRUE(mFileData, NS_ERROR_OUT_OF_MEMORY);
    }

    uint32_t bytesRead = 0;
    mAsyncStream->Read(mFileData + mDataLen, aCount, &bytesRead);

This code verifies there is a buffer if mDataFormat != FILE_AS_ARRAYBUFFER, but does not if its FILE_AS_ARRAYBUFFER.

I don't see where mFileData is ever set for the FILE_AS_ARRAYBUFFER case?  Andrea, how is this supposed to work?

Comment 18

[Andrea Marchesini [:baku]]

https://dxr.mozilla.org/mozilla-central/source/dom/file/FileReader.cpp#424

mFileData is allocated differently if it's going to be used by the JS engine. This happens in FileReader::ReadFileContent:

  if (mDataFormat == FILE_AS_ARRAYBUFFER) {
    mFileData = js_pod_malloc<char>(mTotal);   

mTotal is set a few lines above:

  mTotal = mBlob->GetSize(aRv); 

and just before doing the Read() we do:

    CheckedInt<uint64_t> size = mDataLen;
    size += aCount;

    //Update memory buffer to reflect the contents of the file
    if (!size.isValid() ||
        // PR_Realloc doesn't support over 4GB memory size even if 64-bit OS
        size.value() > UINT32_MAX ||
        size.value() > mTotal) {
      return NS_ERROR_OUT_OF_MEMORY;
    }

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

But don't we try to start reading the stream before that js_pod_malloc()?  This seems racy, but also doesn't stop the stream reading if the js_pod_malloc fails, right?

https://dxr.mozilla.org/mozilla-central/source/dom/file/FileReader.cpp#415

Should we move the js_pod_malloc to be right after the mTotal calculation?

Comment 20

[Andrea Marchesini [:baku]]

Attachment: fr.patch

You are totally right. Thanks for catching this!

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8824025 [details] [diff] [review]
fr.patch

Review of attachment 8824025 [details] [diff] [review]:
-----------------------------------------------------------------

Please call FreeFileData() if the DoAsyncWait() fails.  r=me with the change.  Thanks!

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7af31a6df99
FileReader should start reading data only after allocating the buffer, r=bkelly

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b7af31a6df99

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Andrea, can you request uplift when you get a chance?  Would be great to see if this fixes the beta crashes before next merge.

Comment 25

[Andrea Marchesini [:baku]]

Comment on attachment 8824025 [details] [diff] [review]
fr.patch

Approval Request Comment
[Feature/Bug causing the regression]: FileReader API
[User impact if declined]: a crash can occur.
[Is this code covered by automated tests?]: no, because it's racy to reproduce.
[Has the fix been verified in Nightly?]: hard to verify because racy.
[Needs manual test from QE? If yes, steps to reproduce]: read above.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no.
[Why is the change risky/not risky?]: We just allocate the string before starting to read from the pipe.
[String changes made/needed]: none

Maybe we want this in beta as well.

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8824025 [details] [diff] [review]
fr.patch

This crash fix should land for 51 beta 13 next week.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c23760f94dec

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d78b42d605ca

Comment 29

[[:philipp]]

hi, the fix has gone into 51.0b13, but crash reports with this signature are still continuing for those users: http://bit.ly/2if11Bw
should we reopen the bug?

Comment 30

[Andrea Marchesini [:baku]]

Yes, or maybe file a new bug.