1975674 - Firefox crashes in nsIFile::Contains on Windows when path is null while writing extensions.json

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect, P3)

Description

[Rob Wu [:robwu]]

I investigated a report of a crash in bug 1975255, where the browser allegedly crashed when a profile directory is reused on different operating systems (Windows & Linux), bp-3abf727b-0791-437c-ad6a-1f15e0250610.

I identified a likely root cause for the crash:

• The relativePath getter here calls this.location.dir.contains(this.file), but this.file can be null, at https://searchfox.org/mozilla-central/rev/16abc60460bd25e02750da8842a1e914bc738afb/toolkit/mozapps/extensions/internal/XPIProvider.sys.mjs#542
• Similarly there is a dir.contains(this.get(aId).file) call at https://searchfox.org/mozilla-central/rev/16abc60460bd25e02750da8842a1e914bc738afb/toolkit/mozapps/extensions/internal/XPIProvider.sys.mjs#898
• On Linux/macOS/Android passing null would throw NS_ERROR_ILLEGAL_VALUE (which is NS_ERROR_INVALID_ARG from nsLocalFileUnix::Contains), but such a nullptr check is missing in the Windows implementation, resulting in a nullptr dereference.

We should fall back more gracefully than crashing the browser entirely. This problem is also likely a root cause of bug 1580252, which was closed because nobody could figure out what was happening.

I pinpointed the issue to the relativePath getter mentioned above, because there are very few callers of nsIFile::Contains in the code base, and out of all callers that I audited, only the logic in XPIProvider can pass null potentially. Furthermore, although most stack traces provide little information (with stack traces similar to bug 1580252), some crash reports also include mozilla::dom::IOUtils::WriteJSON on the stack (e.g. bp-6a6ff464-0302-4663-b1e4-22a6b0250613), which is another pointer towards the relativePath getter, because that getter is invoked from toJSON, triggered via IOUtils.writeJSON() call in XPIDatabase (this writes the extensions.json file).

Comment 1

[Rob Wu [:robwu]]

This is a Windows-only crash which I can reproduce by running the following snippet in the global Browser Console: FileUtils.File("C:\\").contains(null)

Results in the following crash: bp-623b9791-4add-4de9-93b6-472c30250703

I'll fix this by adding a nullptr check to https://searchfox.org/mozilla-central/rev/02545fb16ddbc8dae7788c6f52be2c1504b50345/xpcom/io/nsLocalFileWin.cpp#3139

similar to what we already have on other platforms: https://searchfox.org/mozilla-central/rev/02545fb16ddbc8dae7788c6f52be2c1504b50345/xpcom/io/nsLocalFileUnix.cpp#2117

Comment 2

[Rob Wu [:robwu]]

Attachment: Bug 1975674 - Add null check to nsIFile::Contains on Windows

• add basic unit tests, run with: ./mach gtest TestFile.*

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/e39f8c3f3aad
https://hg.mozilla.org/integration/autoland/rev/5c9e47f6c882
Add null check to nsIFile::Contains on Windows r=xpcom-reviewers,mccr8

Comment 4

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/5c9e47f6c882

Comment 5

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

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

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox141 to wontfix.

For more information, please visit BugBot documentation.

Comment 6

[Rob Wu [:robwu]]

Although the patch has minimal risk, the volume of crashes is low, so it doesn't have to be uplifted to Beta. 66% of the crashes are on ESR (ESR128, ESR115, and with low volume ESR140).

Because of the low risk to the patch (changing the hard crash to an error so that Windows behaves like other platforms) coupled with the high severity if encountered (main process crash), I'm inclined to uplift this to all ESR branches.

Comment 7

[Rob Wu [:robwu]]

The above stats are based on crashes in the past 6 months. If I filter to the last month, 115 does not appear.

ESR128 is almost EOL.

So I'm only going to uplift to ESR140.

Comment 8

[Rob Wu [:robwu]]

Attachment: Bug 1975674 - Add null check to nsIFile::Contains on Windows

• add basic unit tests, run with: ./mach gtest TestFile.*

Original Revision: https://phabricator.services.mozilla.com/D256255

Comment 9

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined: Startup crash in rare circumstances on Windows. Affects all products including Firefox and Thunderbird.
• Code covered by automated testing: yes
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: QA not needed, but if desired, STR: https://bugzilla.mozilla.org/show_bug.cgi?id=1975674#c1
• Risk associated with taking this patch: Low
• Explanation of risk level: Changes crash to raising an error, consistent with all non-Windows platforms
• String changes made/needed: None
• Is Android affected?: no

Comment 10

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/b0d90fd627ee