Crash in memcpy | NS_CopySegmentToBuffer rising in Firefox 49

RESOLVED FIXED in Firefox 51

Status

()

Core
DOM
--
critical
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: philipp, Assigned: baku)

Tracking

({crash})

49 Branch
mozilla53
x86
Windows
crash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49+ wontfix, firefox50 fix-optional, firefox51 fixed, firefox52 fixed, firefox53 fixed)

Details

(crash signature)

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
[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

2 years ago
Andrea, any thoughts on this one?  I'm not familiar with FileReader.
Component: Untriaged → DOM
Flags: needinfo?(amarchesini)
Track 49+ as this crash keeps showing in 49.
tracking-firefox49: ? → +
Too late for 49 as we are heading into the RC build next Monday. 1 week till release.
status-firefox49: affected → wontfix
status-firefox50: affected → fix-optional

Comment 4

2 years ago
I'll look at this from a pipe angle next week.
Flags: needinfo?(bkelly)
(Assignee)

Comment 5

2 years ago
We had similar issues when the underlying file changes size. But that should be fixed by now. It could be something similar.
Flags: needinfo?(amarchesini)
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
status-firefox52: --- → affected
Duplicate of this bug: 1314046
Bug 1314046 seems to have a set of steps to reproduce this signature.
(Reporter)

Updated

2 years ago
Keywords: regression
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.
Flags: needinfo?(mozillamarcia.knous)
Flags: needinfo?(madperson)
(Reporter)

Comment 10

2 years ago
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
Flags: needinfo?(madperson)
Keywords: regression
Flags: needinfo?(mozillamarcia.knous)
Perhaps some help in reproducing, we're seeing this more often with the patches applied in bug 1286082, comment 12.
(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.
(Reporter)

Comment 13

a year ago
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.
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?
Component: DOM → XPCOM
Flags: needinfo?(amarchesini)
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.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Flags: needinfo?(bkelly)
Flags: needinfo?(amarchesini)

Updated

a year ago
Keywords: leave-open
Created attachment 8823430 [details] [diff] [review]
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.
Attachment #8823430 - Flags: review?(nfroyd)
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?
Assignee: bkelly → nobody
Status: ASSIGNED → NEW
Component: XPCOM → DOM
Flags: needinfo?(amarchesini)

Updated

a year ago
Attachment #8823430 - Flags: review?(nfroyd)
(Assignee)

Comment 18

a year ago
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;
    }
Flags: needinfo?(amarchesini)
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?
Flags: needinfo?(amarchesini)
(Assignee)

Comment 20

a year ago
Created attachment 8824025 [details] [diff] [review]
fr.patch

You are totally right. Thanks for catching this!
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attachment #8824025 - Flags: review?(bkelly)
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!
Attachment #8824025 - Flags: review?(bkelly) → review+

Comment 22

a year ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7af31a6df99
FileReader should start reading data only after allocating the buffer, r=bkelly

Updated

a year ago
Keywords: leave-open

Comment 23

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b7af31a6df99
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Andrea, can you request uplift when you get a chance?  Would be great to see if this fixes the beta crashes before next merge.
Flags: needinfo?(amarchesini)
(Assignee)

Comment 25

a year ago
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.
Flags: needinfo?(amarchesini)
Attachment #8824025 - Flags: approval-mozilla-beta?
Attachment #8824025 - Flags: approval-mozilla-aurora?
Comment on attachment 8824025 [details] [diff] [review]
fr.patch

This crash fix should land for 51 beta 13 next week.
Attachment #8824025 - Flags: approval-mozilla-beta?
Attachment #8824025 - Flags: approval-mozilla-beta+
Attachment #8824025 - Flags: approval-mozilla-aurora?
Attachment #8824025 - Flags: approval-mozilla-aurora+

Comment 27

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/c23760f94dec
status-firefox52: affected → fixed

Comment 28

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/d78b42d605ca
status-firefox51: affected → fixed
(Reporter)

Comment 29

a year ago
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?
Flags: needinfo?(amarchesini)
(Assignee)

Comment 30

a year ago
Yes, or maybe file a new bug.
Flags: needinfo?(amarchesini)
(Reporter)

Updated

a year ago
Blocks: 1330273
(Assignee)

Updated

a year ago
Blocks: 1331340
(Assignee)

Updated

a year ago
Blocks: 1332587
You need to log in before you can comment on or make changes to this bug.