Closed Bug 1516787 Opened 2 years ago Closed 2 years ago

Crash in mozilla::gl::SharedSurface_ANGLEShareHandle::~SharedSurface_ANGLEShareHandle

Categories

(Core :: Canvas: WebGL, defect)

Unspecified
Windows 10
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox-esr60 --- unaffected
firefox64 --- unaffected
firefox65 + fixed
firefox66 + fixed

People

(Reporter: calixte, Assigned: sotaro)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [post-critsmash-triage])

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is
report bp-8c3eb33c-b613-4d05-9b5b-eb2b30181228.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll void mozilla::gl::SharedSurface_ANGLEShareHandle::~SharedSurface_ANGLEShareHandle gfx/gl/SharedSurfaceANGLE.cpp:101
1 xul.dll void mozilla::gl::SharedSurface_ANGLEShareHandle::~SharedSurface_ANGLEShareHandle gfx/gl/SharedSurfaceANGLE.cpp:100
2 xul.dll void mozilla::layers::SharedSurfaceTextureData::~SharedSurfaceTextureData gfx/layers/client/TextureClientSharedSurface.cpp:26
3 xul.dll void mozilla::layers::SharedSurfaceTextureClient::~SharedSurfaceTextureClient gfx/layers/client/TextureClientSharedSurface.cpp:80
4 xul.dll void mozilla::layers::SharedSurfaceTextureClient::~SharedSurfaceTextureClient gfx/layers/client/TextureClientSharedSurface.cpp:62
5 xul.dll mozilla::AtomicRefCountedWithFinalize<mozilla::layers::TextureClient>::Release gfx/layers/AtomicRefCountedWithFinalize.h:152
6 xul.dll void std::_List_buy<std::pair<const unsigned long long, RefPtr<mozilla::layers::TextureClient> >, std::allocator<std::pair<const unsigned long long, RefPtr<mozilla::layers::TextureClient> > > >::_Freenode vs2017_15.8.4/VC/include/list:728
7 xul.dll class std::_List_iterator<std::_List_val<std::_List_simple_types<std::pair<const unsigned long long, RefPtr<mozilla::layers::TextureClient> > > > > std::_Hash<std::_Umap_traits<unsigned long long, RefPtr<mozilla::layers::TextureClient>, std::_Uhash_compare<unsigned long long, std::hash<unsigned long long>, std::equal_to<unsigned long long> >, std::allocator<std::pair<const unsigned long long, RefPtr<mozilla::layers::TextureClient> > >, 0> >::erase vs2017_15.8.4/VC/include/xhash:607
8 xul.dll void mozilla::layers::CompositorBridgeChild::NotifyNotUsed gfx/layers/ipc/CompositorBridgeChild.cpp:839
9 xul.dll mozilla::layers::CompositorBridgeChild::RecvParentAsyncMessages gfx/layers/ipc/CompositorBridgeChild.cpp:784

=============================================================


There are 4 crashes (from 4 installations) in nightly 66 starting with buildid 20181228093007. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1493497.

[1] https://hg.mozilla.org/mozilla-central/rev?node=41d9d168940d
Flags: needinfo?
Flags: needinfo? → needinfo?(sotaro.ikeda.g)
Group: core-security-release → gfx-core-security
I misunderstood mGL as the mGL is defined as strong reference. It is defined as WeakPtr<GLContext> mGL.
Flags: needinfo?(sotaro.ikeda.g)
Assignee: nobody → sotaro.ikeda.g
And there could be a case that GLContext::MarkDestroyed() is called by WebGLContext.
Attachment #9034336 - Flags: review?(jgilbert)

Blocks a bug needing uplift to 65, so tracking this accordingly.

Attachment #9034336 - Flags: review?(jgilbert) → review+

Comment on attachment 9034336 [details] [diff] [review]
patch - Add GLContextEGL::OnMarkDestroyed()

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: It is not easy. Thre is no specific STR to hit the problem.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No

Which older supported branches are affected by this flaw?: firefox66

If not all supported branches, which bug introduced the flaw?: Bug 1493497

Do you have backports for the affected branches?: No

If not, how different, hard to create, and risky will they be?: It is easy to back port. It is not risky.

How likely is this patch to cause regressions; how much testing does it need?: It is not likely to cause the regression. Current auto tests already cover the code. And there is no specific STR to hit the problem.

Attachment #9034336 - Flags: sec-approval?

The problem directly affect only to firefox66, but it needs to be uplifted to firefox65 because of Bug 1493497.

Comment on attachment 9034336 [details] [diff] [review]
patch - Add GLContextEGL::OnMarkDestroyed()

Clearing sec-approval as this is a sec-moderate now. We probably still want this on Beta so please nominate a beta patch.

Attachment #9034336 - Flags: sec-approval?
Group: gfx-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

Please nominate this for Beta approval.

Flags: needinfo?(sotaro.ikeda.g)

Comment on attachment 9034336 [details] [diff] [review]
patch - Add GLContextEGL::OnMarkDestroyed()

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1493497

User impact if declined: When Bug 1493497 is uplifted to beta and this is not uplifted, it cause crash.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce:

List of other uplifts needed: Bug 1493497

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The change is relatively simple for avoiding nullptr dereference.

String changes made/needed: None

Flags: needinfo?(sotaro.ikeda.g)
Attachment #9034336 - Flags: approval-mozilla-beta?

Comment on attachment 9034336 [details] [diff] [review]
patch - Add GLContextEGL::OnMarkDestroyed()

[Triage Comment]
Follow-up fix for bug 1493497. Approved for 65.0b12.

Attachment #9034336 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.