EXCEPTION_ACCESS_VIOLATION_READ using JS module

RESOLVED FIXED in Firefox 57

Status

()

Core
JavaScript Engine
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: Jun, Assigned: jonco)

Tracking

57 Branch
mozilla57
Points:
---

Firefox Tracking Flags

(firefox-esr52 disabled, firefox55 disabled, firefox56 disabled, firefox57 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

11 months ago
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.101 Safari/537.36

Steps to reproduce:

1. enable dom.moduleScripts.enabled pref 
2. Go to https://test.shhnjk.com/test.html
3 Reload the page


Actual results:

Crash.
https://crash-stats.mozilla.com/report/index/f974d23e-b867-4b19-9741-3db9e0170901


Expected results:

should not crash.

Tested on latest Nightly on Windows 10.

Comment 1

11 months ago
:jonco, seems the pref mentioned in comment #0 was added in a patch you wrote, so I'm hoping you know who can look at this? :-)

Not entirely sure this needs to be sec-sensitive, crash looks like a near-null read to me (but please check, not an expert).
Group: firefox-core-security → core-security
Component: Untriaged → JavaScript Engine
Flags: needinfo?(jcoppeard)
Product: Firefox → Core
Assignee: nobody → jcoppeard
Flags: needinfo?(jcoppeard)
This is not sec-sensitive as JS modules are disabled by default.
Created attachment 8904246 [details] [diff] [review]
bug1395896-mixed-content-import

The problem is that ModuleLoadRequest::ModuleErrored() asserts that the module script is null or errored after the status of all a module's dependencies are checked, but in ScriptLoader::StartFetchingModuleAndDependencies() we can hit an error loading a dependency before we we add that dependency to the list in the load request and hence it appears that the module's dependencies all loaded OK. 

The fix is just to add dependent load requests to the list before we try to load them.
Attachment #8904246 - Flags: review?(bkelly)
Comment on attachment 8904246 [details] [diff] [review]
bug1395896-mixed-content-import

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

Thanks for adding a test!
Attachment #8904246 - Flags: review?(bkelly) → review+
Blocks: 1388728

Updated

11 months ago
Group: core-security → javascript-core-security
Daniel, does this really need to be private given the feature is disabled by default?
Flags: needinfo?(dveditz)
https://hg.mozilla.org/mozilla-central/rev/0be76a0e23a2
Status: UNCONFIRMED → RESOLVED
Last Resolved: 11 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Updated

11 months ago
Group: javascript-core-security → core-security-release
status-firefox55: --- → disabled
status-firefox56: --- → disabled
status-firefox-esr52: --- → disabled
(In reply to Ben Kelly [:bkelly] from comment #5)
> Daniel, does this really need to be private given the feature is disabled by default?

Since it's fixed, no. In general if not fixed it would depend on how confident you are we'd fix a given security bug before enabling the feature.
Group: core-security-release
Flags: needinfo?(dveditz)
You need to log in before you can comment on or make changes to this bug.