Closed Bug 1627667 Opened 4 years ago Closed 4 years ago

Crash in [@ strlen | std::basic_string<T>::assign]

Categories

(Core :: Graphics: CanvasWebGL, defect, P1)

Unspecified
All
defect

Tracking

()

RESOLVED FIXED
82 Branch
Tracking Status
firefox-esr68 --- unaffected
firefox-esr78 81+ fixed
firefox79 --- wontfix
firefox80 --- wontfix
firefox81 + fixed
firefox82 + fixed

People

(Reporter: gsvelto, Assigned: jgilbert)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

This bug is for crash report bp-a786feaf-ea57-4075-970f-8fceb0200404.

Top 10 frames of crashing thread:

0 ucrtbase.dll strlen 
1 xul.dll std::basic_string<char, std::char_traits<char>, std::allocator<char> >::assign vs2017_15.8.4/VC/include/xstring:2676
2 xul.dll mozilla::WebGLContext::GetString const dom/canvas/WebGLContext.cpp
3 xul.dll mozilla::RunOn<mozilla::Maybe<std::basic_string<char, std::char_traits<char>, std::allocator<char> > >  dom/canvas/ClientWebGLContext.cpp:372
4 xul.dll mozilla::ClientWebGLContext::GetParameter dom/canvas/ClientWebGLContext.cpp:1907
5 xul.dll mozilla::dom::WebGLRenderingContext_Binding::getParameter dom/bindings/WebGLRenderingContextBinding.cpp:18104
6 xul.dll mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions> dom/bindings/BindingUtils.cpp:3205
7 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:581
8 xul.dll Interpret js/src/vm/Interpreter.cpp:3040
9 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:616

This is a NULL pointer access that started with 74.0. Disregard all crashes for older versions as they're unrelated and have different stacks. The stacks are consistent across versions and the oldest crash I could find is for 74.0 nightly with buildid 20200116214549.

I could find a handful of Linux crashes too so this isn't Windows-specific. Interestingly on Linux we hit a MOZ_CRASH() instead of seg-faulting.

Crash Signature: [@ strlen | std::basic_string<T>::assign] → [@ strlen | std::basic_string<T>::assign] [@ mozalloc_abort | mozilla::WebGLContext::GetString[abi:cxx11]]
OS: Windows → All

The priority flag is not set for this bug.
:jgilbert, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(jgilbert)

We're calling std::string(nullptr), which is UB.

Severity: normal → S3
Flags: needinfo?(jgilbert)
Priority: -- → P1
Assignee: nobody → jgilbert
Status: NEW → ASSIGNED
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c8da9511671f
Don't use nullptr with std::string. r=lsalzman
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 82 Branch

[Tracking Requested - why for this release]: ~500 crashes last Release cycle

[Tracking Requested - why for this release]: ~500 crashes in Firefox 78 Release cycle, so we expect equivalent crashiness on 78esr.

Comment on attachment 9172293 [details]
Bug 1627667 - Don't use nullptr with std::string.

Beta/Release Uplift Approval Request

  • User impact if declined: ~500 crashes per cycle on Release
  • 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: 78esr
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Straightforward and local patch.
  • String changes made/needed: none
Attachment #9172293 - Flags: approval-mozilla-beta?

Comment on attachment 9172293 [details]
Bug 1627667 - Don't use nullptr with std::string.

Approved for 81.0b4.

Attachment #9172293 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Please nominate this for ESR78 approval when you get a chance.

Flags: needinfo?(jgilbert)

Comment on attachment 9172293 [details]
Bug 1627667 - Don't use nullptr with std::string.

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Medium-volume crash fix
  • User impact if declined: Content process crashes.
  • Fix Landed on Version: 81
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Low risk because simple patch.
  • String or UUID changes made by this patch: none
Flags: needinfo?(jgilbert)
Attachment #9172293 - Flags: approval-mozilla-esr78?

Comment on attachment 9172293 [details]
Bug 1627667 - Don't use nullptr with std::string.

approved for 78.3

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

Attachment

General

Created:
Updated:
Size: