1803109 - Crash in [@ memcpy | mozilla::ProfileBufferEntryWriter::WriteBytes]

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect, P2)

Description

[Calixte Denizet (:calixte)]

Crash report: https://crash-stats.mozilla.org/report/index/220ae205-ee30-4eba-a0bd-6554e0221129

Reason: EXCEPTION_ACCESS_VIOLATION_WRITE

Top 10 frames of crashing thread:

0  VCRUNTIME140.dll  memcpy  D:\a\_work\1\s\src\vctools\crt\vcruntime\src\string\amd64\memcpy.asm:442
1  xul.dll  mozilla::ProfileBufferEntryWriter::WriteBytes  mozglue/baseprofiler/public/ProfileBufferEntrySerialization.h:532
2  xul.dll  mozilla::ProfileChunkedBuffer::PutObjects<mozilla::ProfileBufferEntryKind, mozilla::MarkerOptions, mozilla::ProfilerStringView<char>, mozilla::MarkerCategory, unsigned char, mozilla::MarkerPayloadType, mozilla::ProfilerStringView<char> >::<lambda_2>::operator const  mozglue/baseprofiler/public/ProfileChunkedBuffer.h:363
2  xul.dll  mozilla::ProfileChunkedBuffer::ReserveAndPut<`lambda at /builds/worker/workspace/obj-build/dist/include/mozilla/ProfileChunkedBuffer.h:358:9', `lambda at /builds/worker/workspace/obj-build/dist/include/mozilla/ProfileChunkedBuffer.h:359:9'>::<lambda_2>::operator const  mozglue/baseprofiler/public/ProfileChunkedBuffer.h:327
2  xul.dll  mozilla::ProfileChunkedBuffer::ReserveAndPutRaw<`lambda at /builds/worker/workspace/obj-build/dist/include/mozilla/ProfileChunkedBuffer.h:317:9', `lambda at /builds/worker/workspace/obj-build/dist/include/mozilla/ProfileChunkedBuffer.h:322:9'>  mozglue/baseprofiler/public/ProfileChunkedBuffer.h:1200
3  xul.dll  mozilla::ProfileChunkedBuffer::ReserveAndPut<`lambda at /builds/worker/workspace/obj-build/dist/include/mozilla/ProfileChunkedBuffer.h:358:9', `lambda at /builds/worker/workspace/obj-build/dist/include/mozilla/ProfileChunkedBuffer.h:359:9'>  mozglue/baseprofiler/public/ProfileChunkedBuffer.h:316
4  xul.dll  mozilla::ProfileChunkedBuffer::PutObjects  mozglue/baseprofiler/public/ProfileChunkedBuffer.h:357
4  xul.dll  mozilla::base_profiler_markers_detail::StreamFunctionTypeHelper<void   mozglue/baseprofiler/public/BaseProfilerMarkersDetail.h:119
4  xul.dll  mozilla::base_profiler_markers_detail::MarkerTypeSerialization<mozilla::baseprofiler::markers::TextMarker>::Serialize<nsPrintfCString>  mozglue/baseprofiler/public/BaseProfilerMarkersDetail.h:165
5  xul.dll  mozilla::base_profiler_markers_detail::AddMarkerWithOptionalStackToBuffer  mozglue/baseprofiler/public/BaseProfilerMarkersDetail.h:254

STR:

• Download the attachment: https://bugzilla.mozilla.org/attachment.cgi?id=9305743
• open a new tab and start the profiler
• copy/paste the url of the downloaded file in the address bar and hit return
• crash.

The html contains an iframe with a src="data:application/pdf;...." which is 5M long.

Comment 1

[Julien Wajsberg [:julienw]]

Simpler STR:

1. start the profiler.
2. click on https://bugzilla.mozilla.org/attachment.cgi?id=9305743
3. wait a bit => crash

Comment 2

[Calixte Denizet (:calixte)]

I suppose it's because we don't check if tail is lower than the span size:
https://searchfox.org/mozilla-central/rev/8a4f55bc09ffc5c25dcb4586c51ae4a9fee77b4c/mozglue/baseprofiler/public/ProfileBufferEntrySerialization.h#532-534

Comment 3

[Julien Wajsberg [:julienw]]

Yeah it would be good to check about this at this location indeed.

Probably we generate too long strings, such as:

• https://searchfox.org/mozilla-central/rev/8a4f55bc09ffc5c25dcb4586c51ae4a9fee77b4c/layout/base/nsPresContext.cpp#2736-2738 (an iframe with a long data url)
• https://searchfox.org/mozilla-central/rev/8a4f55bc09ffc5c25dcb4586c51ae4a9fee77b4c/dom/base/nsDOMNavigationTiming.cpp#460-466 (an iframe with a long data url)
• https://searchfox.org/mozilla-central/rev/8a4f55bc09ffc5c25dcb4586c51ae4a9fee77b4c/layout/base/nsRefreshDriver.cpp#1549-1555 (an <img> tag with a long data url)
• and probably other places

Comment 4

[Julien Wajsberg [:julienw]]

I restricted the bug because this seems to come from the size of URLs, so this is user-controlled. The profiler needs to be running so this shouldn't impact the average user, but this could impact web developers and firefox engineers. A possible scenario is that a malicious user would file a performance problem bug with a website having crafted URLs, that would target firefox performance engineers.

Comment 5

[[:sergesanspaille]]

(In reply to Calixte Denizet (:calixte) from comment #2)

I suppose it's because we don't check if tail is lower than the span size:
https://searchfox.org/mozilla-central/rev/8a4f55bc09ffc5c25dcb4586c51ae4a9fee77b4c/mozglue/baseprofiler/public/ProfileBufferEntrySerialization.h#532-534

That's my understanding too. But in that case it would have hit the "MOZ_RELEASE_ASSERT(aBytes <= RemainingBytes());" condition, right?

Comment 6

[Calixte Denizet (:calixte)]

Yes, after a quick glance, it seems that the assertion should have been hit.

Comment 7

[Julien Wajsberg [:julienw]]

Pernosco session => https://pernos.co/debug/8O6yQYT0-ZySwbz1FZt_XQ/index.html#f{m[BCAI,AA_,t[AQg,JJuV_,f{e[BCAB,BPU_,s{af53boAAA,bAZc,uxlKn,oyc6u___/

Comment 8

[Julien Wajsberg [:julienw]]

This may not happen anymore thanks to bug 1810055, but the root cause of tail being not checked properly is still there.

Comment 9

[Julien Wajsberg [:julienw]]

My big interrogation is this comment:

      // This should only happen at most once: Only for double spans, and when
      // data crosses the gap or starts there.

It would be good to understand why Gerald thought this when he wrote this code. It's clearly not true nowadays.

My rough guess is that when he wrote this code, markers weren't stored in the main buffer yet, and we were much more controlling the data going in the buffer. Nowadays with the Markers 2.0 work, any data of any size can be stored. Therefore we shouldn't blindly consider that tail will fit in one chunk, instead we should cut it in several chunks until it's completely consumed.

Comment 10

[Julien Wajsberg [:julienw]]

I found this piece of information about the spans: https://searchfox.org/mozilla-central/rev/a8187e40b492dff78e3d3225e652cc06f447484b/mozglue/baseprofiler/public/ProfileBufferEntrySerialization.h#208-227

I still don't know why gerald thought we couldn't have more than 2 spans.

Comment 11

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

My bet is that it was the most straightforward option that works most of the time and Gerald thought that there wouldn't be an object of data that is longer than 10kb.

This is where we are creating the spans and putting them inside the entry writer: https://searchfox.org/mozilla-central/rev/a8187e40b492dff78e3d3225e652cc06f447484b/mozglue/baseprofiler/public/ProfileChunkedBuffer.h#1112-1148
Here's the construction for entry writer there: https://searchfox.org/mozilla-central/rev/a8187e40b492dff78e3d3225e652cc06f447484b/mozglue/baseprofiler/public/ProfileChunkedBuffer.h#1138-1141
mem0 and mem1 are the two spans that we create for these differeent chunks.

The tricky part is that you need to give the spans before you start writing objects on it. Also changing only the ProfileBufferEntryWriter is not enough, we need to also change the ProfileBufferEntryReader too.

This is the part where we put the second span for the entry reader: https://searchfox.org/mozilla-central/rev/a8187e40b492dff78e3d3225e652cc06f447484b/mozglue/baseprofiler/public/ProfileChunkedBufferDetail.h#262-284

Comment 12

[Julien Wajsberg [:julienw]]

Hey Gerald, I hope everything's fine on your side!
We were wondering if you could kindly confirm some our guesses here? Thanks a ton :-)

Comment 13

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Hi! 👋 Sorry for the trouble I left.
I think the main issue is that ReserveAndPutRaw allocates the 2nd span without checking if there is enough space (in this case >4MB in a 1MB chunk), so it happily creates a span that overruns the chunk.
I don't remember why I assumed it would be okay, though I have a vague recollection that in the past there was a check somewhere that we couldn't reserve anything as big as a single chunk; maybe I mistakenly removed it during some past re-architecting.

Anyway, what to do now?

I think that at this else before the 2-span case we should check that blockBytes < current->BufferBytes(), and if not, give up and do the usual mFailedPutBytes += blockBytes; (see other failure cases below).
This would silently discard too-long objects.

And separately, these "dangerous" markers should probably automatically snip over-long strings to avoid storing megabytes of data (it seems doubtful it would ever be useful!?)

Feel free to ping me again if you need further help. Good luck!

Comment 14

[Kash Shampur [:kshampur] ⌚EST]

Thanks for the insight Gerald!
I tried this on an old revision where this crash still exists and this seemed to stop crashing.

Comment 15

[Kash Shampur [:kshampur] ⌚EST]

Attachment: Bug 1803109 - Discard blocks of data that are too big for two chunks. r=canaltinova

Currently, ReserveAndPutRaw allocates a second span even if the data would still overrun the allocated memory.
Here a second conditional is added to check if the blockBytes >= BufferBytes and discard the data if so as
it would overrun the two spans.

Comment 16

[Kash Shampur [:kshampur] ⌚EST]

Comment on attachment 9312864 [details]
Bug 1803109 - Discard blocks of data that are too big for two chunks. r=canaltinova

Security Approval Request

• How easily could an exploit be constructed based on the patch?: After bug 1810055 landed, many instances of this was fixed, but there may exist a few more.
  We think this would be difficult to exploit because of where/how the chunk is created and written to. The profiler also needs to be active for this to be exploited.
  A second opinion would be valuable from Security team
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: ESR, release
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: it would be easy to create, minimal line changes
• How likely is this patch to cause regressions; how much testing does it need?: very unlikely
• Is Android affected?: Yes

Comment 17

[Julien Wajsberg [:julienw]]

BTW the patch has been pushed to try (my fault). Hopefully that's not too bad because it's sec-moderate, but wanted to mention it still.

Comment 18

[Kash Shampur [:kshampur] ⌚EST]

Attachment: Bug 1803109 - Add test for checking very long strings in profiler markers. r=canaltinova

Comment 19

[Tom Ritter [:tjr]]

Comment on attachment 9312864 [details]
Bug 1803109 - Discard blocks of data that are too big for two chunks. r=canaltinova

If the profiler needs to be active, then I'm okay with landing this patch and the test together.

Comment 20

[Julien Cristau [:jcristau]]

Discard blocks of data that are too big for two chunks. r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/adcb31b93a01
https://hg.mozilla.org/mozilla-central/rev/adcb31b93a01
Add test for checking very long strings in profiler markers. r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/7cb0464558a6
https://hg.mozilla.org/mozilla-central/rev/7cb0464558a6

Comment 21

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

The patch landed in nightly and beta is affected.
:kshampur, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox110 to wontfix.

For more information, please visit auto_nag documentation.

Comment 22

[Kash Shampur [:kshampur] ⌚EST]

Since profiler needs to be active for this scenario, I think this does not require an uplift.

Comment 23

[Kash Shampur [:kshampur] ⌚EST]

ah oops forgot to do that, thanks :RyanVM!