1888333 - Double-free in [@ SkRefCntBase::unref]

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Paul Bone [:pbone]]

Crash report: https://crash-stats.mozilla.org/report/index/8dd342a4-8f6c-401a-a9bb-a7ab60240308

Reason: SIGSEGV / SEGV_ACCERR

Top 10 frames of crashing thread:

0  libxul.so  std::__atomic_base<int>::fetch_add  /usr/include/c++/11/bits/atomic_base.h:618
0  libxul.so  SkRefCntBase::unref const  /build/firefox/parts/firefox/build/gfx/skia/skia/include/core/SkRefCnt.h:75
0  libxul.so  SkSafeUnref<SkShader>  /build/firefox/parts/firefox/build/gfx/skia/skia/include/core/SkRefCnt.h:151
0  libxul.so  sk_sp<SkShader>::~sk_sp  /build/firefox/parts/firefox/build/gfx/skia/skia/include/core/SkRefCnt.h:256
0  libxul.so  SkPaint::~SkPaint  /build/firefox/parts/firefox/build/gfx/skia/skia/src/core/SkPaint.cpp:60
1  libxul.so  mozilla::gfx::DrawTargetSkia::Mask  /build/firefox/parts/firefox/build/gfx/2d/DrawTargetSkia.cpp:1424
2  libxul.so  mozilla::gfx::DrawTargetWebgl::DrawRectFallback  /build/firefox/parts/firefox/build/dom/canvas/DrawTargetWebgl.cpp:1832
3  libxul.so  mozilla::gfx::DrawTargetWebgl::DrawRect  /build/firefox/parts/firefox/build/dom/canvas/DrawTargetWebgl.cpp:1814
4  libxul.so  mozilla::gfx::DrawTargetWebgl::MaskSurface  /build/firefox/parts/firefox/build/dom/canvas/DrawTargetWebgl.cpp:3723
5  libxul.so  mozilla::gfx::RecordedMaskSurface::PlayEvent const  /build/firefox/parts/firefox/build/gfx/2d/RecordedEventImpl.h:4069

This is a PHC-annotated crash, it is an access into a field 8 bytes into an 80byte structure allocated at: 0x7f1e61afbfb0 It was allocated at:

#0    malloc (firefox)
#1    moz_xmalloc (firefox)
#2    sk_make_sp<SkImageShader, sk_sp<SkImage>, SkRect const&, SkTileMode&, SkTileMode&, SkSamplingOptions const&, bool, bool&>(sk_sp<SkImage>, SkRect const&, SkTileMode&, SkTileMode&, SkSamplingOptions const&, bool, bool&) (libxul.so)
#3    SkLocalMatrixShader::MakeWrapped<SkImageShader, sk_sp<SkImage>, SkRect const&, SkTileMode&, SkTileMode&, SkSamplingOptions const&, bool, bool&>(SkMatrix const*, sk_sp<SkImage>, SkRect const&, SkTileMode&, SkTileMode&, SkSamplingOptions const&, bool, bool&) (libxul.so)
#4    SkImageShader::MakeSubset(sk_sp<SkImage>, SkRect const&, SkTileMode, SkTileMode, SkSamplingOptions const&, SkMatrix const*, bool) (libxul.so)
#5    SkImageShader::Make(sk_sp<SkImage>, SkTileMode, SkTileMode, SkSamplingOptions const&, SkMatrix const*, bool) (libxul.so)
#6    SkImage::makeShader(SkTileMode, SkTileMode, SkSamplingOptions const&, SkMatrix const&) const (libxul.so)
#7    mozilla::gfx::SetPaintPattern(SkPaint&, mozilla::gfx::Pattern const&, mozilla::Maybe<mozilla::detail::BaseAutoLock<mozilla::Mutex&> >&, float, SkMatrix const*, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, float> const*) (libxul.so)
#8    mozilla::gfx::DrawTargetSkia::Mask(mozilla::gfx::Pattern const&, mozilla::gfx::Pattern const&, mozilla::gfx::DrawOptions const&) (libxul.so)
#9    mozilla::gfx::DrawTargetWebgl::DrawRectFallback(mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, float> const&, mozilla::gfx::Pattern const&, mozilla::gfx::DrawOptions const&, mozilla::Maybe<mozilla::gfx::DeviceColor>, bool, bool, mozilla::gfx::StrokeOptions const*) (libxul.so)
#10    mozilla::gfx::DrawTargetWebgl::DrawRect(mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, float> const&, mozilla::gfx::Pattern const&, mozilla::gfx::DrawOptions const&, mozilla::Maybe<mozilla::gfx::DeviceColor>, RefPtr<mozilla::gfx::TextureHandle>*, bool, bool, bool, bool, mozilla::gfx::StrokeOptions const*) (libxul.so)
#11    mozilla::gfx::DrawTargetWebgl::MaskSurface(mozilla::gfx::Pattern const&, mozilla::gfx::SourceSurface*, mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits, float>, mozilla::gfx::DrawOptions const&) (libxul.so)
#12    mozilla::gfx::RecordedMaskSurface::PlayEvent(mozilla::gfx::Translator*) const (libxul.so)
#13    mozilla::gfx::RecordedEvent::DoWithEvent<mozilla::gfx::MemReader>(mozilla::gfx::MemReader&, mozilla::gfx::RecordedEvent::EventType, std::function<bool (mozilla::gfx::RecordedEvent*)> const&) (libxul.so)
#14    mozilla::layers::CanvasTranslator::TranslateRecording() (libxul.so)
#15    mozilla::detail::RunnableMethodImpl<mozilla::layers::CanvasTranslator*, void (mozilla::layers::CanvasTranslator::*)(), true, (mozilla::RunnableKind)0, >::Run() (libxul.so)

And freed at

#0    free (firefox)
#1    mozilla::gfx::DrawTargetSkia::Mask(mozilla::gfx::Pattern const&, mozilla::gfx::Pattern const&, mozilla::gfx::DrawOptions const&) (libxul.so)
#2    mozilla::gfx::DrawTargetWebgl::DrawRectFallback(mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, float> const&, mozilla::gfx::Pattern const&, mozilla::gfx::DrawOptions const&, mozilla::Maybe<mozilla::gfx::DeviceColor>, bool, bool, mozilla::gfx::StrokeOptions const*) (libxul.so)
#3    mozilla::gfx::DrawTargetWebgl::DrawRect(mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, float> const&, mozilla::gfx::Pattern const&, mozilla::gfx::DrawOptions const&, mozilla::Maybe<mozilla::gfx::DeviceColor>, RefPtr<mozilla::gfx::TextureHandle>*, bool, bool, bool, bool, mozilla::gfx::StrokeOptions const*) (libxul.so)
#4    mozilla::gfx::DrawTargetWebgl::MaskSurface(mozilla::gfx::Pattern const&, mozilla::gfx::SourceSurface*, mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits, float>, mozilla::gfx::DrawOptions const&) (libxul.so)
#5    mozilla::gfx::RecordedMaskSurface::PlayEvent(mozilla::gfx::Translator*) const (libxul.so)
#6    mozilla::gfx::RecordedEvent::DoWithEvent<mozilla::gfx::MemReader>(mozilla::gfx::MemReader&, mozilla::gfx::RecordedEvent::EventType, std::function<bool (mozilla::gfx::RecordedEvent*)> const&) (libxul.so)
#7    mozilla::layers::CanvasTranslator::TranslateRecording() (libxul.so)
#8    mozilla::detail::RunnableMethodImpl<mozilla::layers::CanvasTranslator*, void (mozilla::layers::CanvasTranslator::*)(), true, (mozilla::RunnableKind)0, >::Run() (libxul.so)
#9    NS_ProcessNextEvent(nsIThread*, bool) (libxul.so)
#10    mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) (libxul.so)
#11    MessageLoop::Run() (libxul.so)
#12    nsThread::ThreadFunc(void*) (libxul.so)
#13    _pt_root (libnspr4.so)
#14    set_alt_signal_stack_and_start(PthreadCreateParams*) (firefox)
#15    start_thread (libc.so.6)

All three PHC-annotated crashes were found on Linux OSs.

Comment 1

[Lee Salzman [:lsalzman]]

Attachment: Bug 1888333. r?aosmond

Comment 2

[Lee Salzman [:lsalzman]]

Comment on attachment 9393739 [details]
Bug 1888333. r?aosmond

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Unsure. It's just a double deref/free of a transient object.
• 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 branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: 114+
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 3

[Lee Salzman [:lsalzman]]

Comment on attachment 9393739 [details]
Bug 1888333. r?aosmond

Beta/Release Uplift Approval Request

• User impact if declined: Potential double-free when rasterizing.
• 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: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Just a change to increase the ref-count to avoid double-free.
• String changes made/needed:
• Is Android affected?: Yes

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined:
• Fix Landed on Version: 126
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):

Comment 4

[Tom Ritter [:tjr] (OOTO until April)]

Comment on attachment 9393739 [details]
Bug 1888333. r?aosmond

Approved to land.

Has the fix been verified in Nightly?: Yes

I think this is a mistake since it hasn't landed yet...

Comment 5

[Dianna Smith [:diannaS]]

Comment on attachment 9393739 [details]
Bug 1888333. r?aosmond

We will not be taking this sec bug in the next planned dot release.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4120fb3d12f5
r=aosmond

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4120fb3d12f5

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9393739 [details]
Bug 1888333. r?aosmond

Approved for 125.0b7.

Comment 9

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/207b2f8e095a

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9393739 [details]
Bug 1888333. r?aosmond

Approved for 115.10esr.

Comment 11

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/a7f6ce840b81