Closed Bug 1686825 Opened 9 months ago Closed 9 months ago

Null deref crash in [@ XPCNativeInterface::NewInstance]

Categories

(Core :: XPConnect, defect, P2)

Unspecified
Windows 10
defect

Tracking

()

RESOLVED FIXED
86 Branch
Tracking Status
firefox-esr78 --- fixed
firefox84 --- wontfix
firefox85 --- wontfix
firefox86 --- fixed

People

(Reporter: mccr8, Assigned: nika)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

Crash report: https://crash-stats.mozilla.org/report/index/460c2944-11cd-4796-972e-292230210114

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0 xul.dll static XPCNativeInterface::NewInstance js/xpconnect/src/XPCWrappedNativeInfo.cpp:348
1 xul.dll static XPCNativeInterface::GetNewOrUsed js/xpconnect/src/XPCWrappedNativeInfo.cpp:141
2 xul.dll static XPCConvert::NativeData2JS js/xpconnect/src/XPCConvert.cpp:355
3 xul.dll nsXPCWrappedJS::CallMethod js/xpconnect/src/XPCWrappedJSClass.cpp:922
4 xul.dll PrepareAndDispatch xpcom/reflect/xptcall/md/win32/xptcstubs_x86_64.cpp:168
5 xul.dll SharedStub 
6 xul.dll mozilla::dom::ServiceWorkerRegistrar::ProfileStarted dom/serviceworkers/ServiceWorkerRegistrar.cpp:1115
7 xul.dll mozilla::dom::ServiceWorkerRegistrar::Observe dom/serviceworkers/ServiceWorkerRegistrar.cpp:1269
8 xul.dll nsObserverList::NotifyObservers xpcom/ds/nsObserverList.cpp:70
9 xul.dll nsObserverService::NotifyObservers xpcom/ds/nsObserverService.cpp:287

The crash is on the last line here:

    const char* bytes = aInfo->Name();
    if (nullptr == bytes ||
        nullptr == (str = JS_AtomizeAndPinString(cx, bytes))) {
      failed = true;
    }
    interfaceName = PropertyKey::fromPinnedString(str);

I think the interfaceName assignment should be behind an else on that if statement. The other calls to JS::PropertyKey::fromPinnedString look like they null check their arguments.

The null deref crashes are only a small percentage of the crashes with this signature, so it doesn't seem too critical, but we might as well make an easy fix where we can.

Priority P2 for an easy fix for a real crash.

Severity: -- → S3
Priority: -- → P2
Assignee: nobody → nika
Status: NEW → ASSIGNED

Comment on attachment 9198259 [details]
Bug 1686825 - Don't perform PropertyKey::fromPinnedString if the string creation failed,

Beta/Release Uplift Approval Request

  • User impact if declined: Low-frequency crash
  • Is this code covered by automated tests?: No
  • 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): Small obvious change to avoid low-frequency crash
  • String changes made/needed: None

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Extremely low risk
  • User impact if declined: Low frequency crash
  • Fix Landed on Version:
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Small obvious change to avoid low-frequency crash
  • String or UUID changes made by this patch: None
Attachment #9198259 - Flags: approval-mozilla-esr78?
Attachment #9198259 - Flags: approval-mozilla-beta?
Pushed by nlayzell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/67e673da78c5
Don't perform PropertyKey::fromPinnedString if the string creation failed, r=pbone
Blocks: 1688120
See Also: → 1688120
Attachment #9198259 - Flags: approval-mozilla-beta? → approval-mozilla-release?
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → 86 Branch

Comment on attachment 9198259 [details]
Bug 1686825 - Don't perform PropertyKey::fromPinnedString if the string creation failed,

Crash volume is really low, I think this can ride to 86 and 78.8.

Attachment #9198259 - Flags: approval-mozilla-release?
Attachment #9198259 - Flags: approval-mozilla-release-
Attachment #9198259 - Flags: approval-mozilla-esr78?
Attachment #9198259 - Flags: approval-mozilla-esr78+
You need to log in before you can comment on or make changes to this bug.