1745239 - Crash in [@ nsExpatDriver::ConsumeToken]

Status: RESOLVED FIXED

(Core :: Security: RLBox, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Maybe Fission related. (DOMFissionEnabled=1)

Crash report: https://crash-stats.mozilla.org/report/index/d1420ff2-2e86-49e6-bdb0-1f0600211209

MOZ_CRASH Reason: RLBox crash: Called memcpy for memory larger than the sandbox

Top 10 frames of crashing thread:

0 libxul.so nsExpatDriver::ConsumeToken parser/htmlparser/nsExpatDriver.cpp:1240
1 libxul.so {virtual override thunk} 
2 libxul.so nsParser::ResumeParse parser/htmlparser/nsParser.cpp:955
3 libxul.so nsParser::OnDataAvailable parser/htmlparser/nsParser.cpp:1306
4 libxul.so {virtual override thunk} 
5 libxul.so mozilla::dom::DOMParser::ParseFromStream dom/base/DOMParser.cpp:229
6 libxul.so mozilla::dom::DOMParser::ParseFromString dom/base/DOMParser.cpp:100
7 libxul.so mozilla::dom::DOMParser_Binding::parseFromString dom/bindings/DOMParserBinding.cpp:96
8 libxul.so bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions> dom/bindings/BindingUtils.cpp:3306
9 libxul.so Interpret js/src/vm/Interpreter.cpp:3243

nsExpatDriver::WillBuildModel: bp-41bec3dd-dcb5-4938-9ab3-6456f0211209

Please check if nsParser::OnStartRequest is also triggered by this, e.g. b7c94820-c74a-4e0a-af3d-c84e40211209 - there are reports for https://register.eshram.gov.in/#/csc/form/uan-card triggering it but the host doesn't load here.

Comment 1

[Bobby Holley (:bholley)]

I'm looking at this.

Comment 2

[Bobby Holley (:bholley)]

I wrote a quick patch with Peter's help to chunk things (eventually just to 4MB, but modified the patch to chunk at 1024 characters to flush out bugs).

https://treeherder.mozilla.org/jobs?repo=try&revision=03da5050b1729970037301a3a6cb36847ee77e89

Still some failures in there. Peter, do you have cycles to have a look?

Comment 3

[Peter Van der Beken [:peterv]]

I'm also looking, but haven't found the issue yet.

Comment 4

[Peter Van der Beken [:peterv]]

Improved patch: https://treeherder.mozilla.org/jobs?repo=try&revision=5d909f42f8c7fab6cd43b6e85ea4a537e6863111

Comment 5

[Bobby Holley (:bholley)]

Thanks Peter! I'll assign this over to you, feel free to steal authorship, upload the patch, and mark me as the reviewer.

I've been thinking a bit about the chunking threshold. Upon reflection, I think it's probably sub-optimal to go as large as the sandbox supports, because it will result in committing a bunch of pages that we could have avoided (i.e. in the 64-bit case, I don't think we actually want to grow the sandbox up to 2GB). I also think we probably want to continue to exercise the chunking logic in non-exceptional cases by keeping the threshold artificially small in debug builds.

So maybe we set the threshold to 1024 bytes in debug builds and 1MB in release builds, and nix the part where we introspect the sandbox?

Comment 6

[Bobby Holley (:bholley)]

And to be explicit, I think the upward pressure on the threshold is to amortize the overhead of ParseBuffer and resuming from within expat. Though if that's actually pretty low, we might benefit from an even smaller threshold in release builds due to better cache locality (though still probably not as low as 1024 bytes).

Comment 7

[Dianna Smith [:diannaS]]

:Peter, is there an update on this patch? The crash volume is high beta and I would rather not ship 96 with this. Are there alternatives to prevent the crashes if this won't be ready in time?

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: Bug 1745239 - Chunk XML parsing to 64k characters at a time. r?bholley!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c1698f123fa
Chunk XML parsing to 64k characters at a time. r=bholley

Comment 10

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/3c1698f123fa

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 9256218 [details]
Bug 1745239 - Chunk XML parsing to 64k characters at a time. r?bholley!

Beta/Release Uplift Approval Request

• User impact if declined: Crashes.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch modifies the chunking of the stream we pass to the XML parsing. If it was broken it would likely be completely broken, and XML parsing is exercised enough in CI that we'd have noticed by now.
• String changes made/needed:

Comment 12

[Dianna Smith [:diannaS]]

Comment on attachment 9256218 [details]
Bug 1745239 - Chunk XML parsing to 64k characters at a time. r?bholley!

Approved for 96.0b8

Comment 13

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2331432d0206

Comment 14

[Dianna Smith [:diannaS]]

Looks like [@ nsExpatDriver::ConsumeToken] was fixed in 96.0b8 but [@ nsExpatDriver::WillBuildModel] is still occurring

Comment 15

[Bobby Holley (:bholley)]

(In reply to Dianna Smith [:diannaS] from comment #14)

Looks like [@ nsExpatDriver::ConsumeToken] was fixed in 96.0b8 but [@ nsExpatDriver::WillBuildModel] is still occurring

Thanks, I didn't look closely enough before to realize there were two different failure modes. Filed bug 1747514 to track the one in WillBuildModel.