Multiple "ASSERTION: nsTDependentString must wrap only null-terminated strings. ... " while profiling with WebRender

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: brennie, Assigned: brennie)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67+ fixed)

Details

Attachments

(1 attachment)

Assignee

Description

3 months ago

While profiling with WebRender enabled on a debug build, multiple of the following assertions occur:

[Parent 5439, Renderer] ###!!! ASSERTION: nsTDependentString must wrap only null-terminated strings. You are probably looking for nsTDependentSubstring.: 'this->mData[substring_type::mLength] == 0', file /home/barret/Workspace/src/hg.mozilla.org/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/nsTString.h, line 456

STR:

  1. Build a debug build of Firefox.
  2. Enable Web Render.
  3. Start profiling with the Gecko Profiler.

Expected results:
We should not be hitting this assertion.

After some investigation, this occurs because <GeckoProfilerHooks as ProfilerHooks>::add_text_marker in the WebRender bindings accepts a &str and passes its pointer and length to gecko_profiler_add_text_marker, which constructs a nsTDependentString. However, Rust strings are not NUL-terminated.

Assignee

Comment 1

3 months ago

gecko_profiler_add_text_marker was being passed a character pointer and a
length to construct a nsDependentCString. However, these values were coming
from a Rust &str, which is not null-terminated, causing an debug assertion to
be hit (and possible memory safety issues if mishandle the string). We now
construct an nsDependentCSubstring instead.

[Tracking Requested - why for this release]:

We should uplift this to 67, otherwise we have a potential out-of-bounds read with the profiler enabled.

Assignee

Updated

3 months ago

Comment 3

3 months ago

Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fd2b7f779973
Fix nsTDependentString with non-null-terminated buffer assertion r=bholley

Keywords: checkin-needed
Assignee

Updated

3 months ago
Keywords: leave-open

Tracking for 67, please request an uplift when you get a chance, thanks.

Assignee

Comment 6

3 months ago

Comment on attachment 9052007 [details]
Bug 1536466 - Fix nsTDependentString with non-null-terminated buffer assertion r?bholley

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: Bug 1532810
  • User impact if declined: Users with WebRender enabled that also are using the Gecko Profiler might experience crashes while profiling
  • Is this code covered by automated tests?: Unknown
  • 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): This is not risky because it touches a codepath that is only hit while users are running with Web Render enabled and collecting a profile with the Gecko Profiler. Even then, the change is very small in scope and well understood.
  • String changes made/needed:
Attachment #9052007 - Flags: approval-mozilla-beta?

Comment on attachment 9052007 [details]
Bug 1536466 - Fix nsTDependentString with non-null-terminated buffer assertion r?bholley

Minimal patch to prevent crashes in webrender / gecko profiler interactions, approved for the next beta, thanks.

Attachment #9052007 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee

Updated

3 months ago
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.