1686825 - Null deref crash in [@ XPCNativeInterface::NewInstance]

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P2)

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Andrew McCreight [:mccr8]]

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.

Comment 2

[Chris Peterson [:cpeterson]]

Priority P2 for an easy fix for a real crash.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1686825 - Don't perform PropertyKey::fromPinnedString if the string creation failed,

Comment 4

[Nika Layzell [:nika] (ni? for response)]

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/67e673da78c5
Don't perform PropertyKey::fromPinnedString if the string creation failed, r=pbone

Comment 6

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/67e673da78c5

Comment 7

[Julien Cristau [:jcristau]]

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.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/47137fbd4294