Closed Bug 1395896 Opened 3 years ago Closed 3 years ago

EXCEPTION_ACCESS_VIOLATION_READ using JS module

Categories

(Core :: JavaScript Engine, defect)

57 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- disabled
firefox55 --- disabled
firefox56 --- disabled
firefox57 --- fixed

People

(Reporter: s.h.h.n.j.k, Assigned: jonco)

References

Details

Attachments

(1 file)

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.
: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.
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
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
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Group: javascript-core-security → core-security-release
(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.