1400399 - crash near null and potential UAF in [@ PLDHashTable::Add]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P2)

Description

[Tyson Smith [:tsmith]]

Attachment: test_case.html

The attached test case requires the fuzzPriv extension.

Found with mozilla-esr 20170914-afdce00aacb4. I have been unable to reproduce this with a m-c build.
Note: This seems to reproduce best on a debug build.

==36324==ERROR: AddressSanitizer: SEGV on unknown address 0x00000000005c (pc 0x7ff19894ac85 bp 0x7fff5eb0f940 sp 0x7fff5eb0f940 T0)
    #0 0x7ff19894ac84 in std::__atomic_base<unsigned int>::load(std::memory_order) const /home/worker/workspace/build/src/clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.8.5/../../../../include/c++/4.8.5/bits/atomic_base.h:496:9
    #1 0x7ff198ac0c7c in Checker::IsWritable() const /home/worker/workspace/build/src/obj-firefox/dist/include/PLDHashTable.h:98:38
    #2 0x7ff198ad2fad in Checker::StartWriteOp() /home/worker/workspace/build/src/obj-firefox/dist/include/PLDHashTable.h:130:5
    #3 0x7ff198abfb37 in PLDHashTable::Add(void const*, mozilla::fallible_t const&) /home/worker/workspace/build/src/xpcom/glue/PLDHashTable.cpp:531:15
    #4 0x7ff19ebfe8e4 in nsBaseHashtable<nsPtrHashKey<nsIDocument const>, RefPtr<mozilla::a11y::DocAccessible>, mozilla::a11y::DocAccessible*>::Put(nsIDocument const*, mozilla::a11y::DocAccessible* const&, mozilla::fallible_t const&) /home/worker/workspace/build/src/obj-firefox/dist/include/nsBaseHashtable.h:150:22
    #5 0x7ff19ebe8a85 in nsBaseHashtable<nsPtrHashKey<nsIDocument const>, RefPtr<mozilla::a11y::DocAccessible>, mozilla::a11y::DocAccessible*>::Put(nsIDocument const*, mozilla::a11y::DocAccessible* const&) /home/worker/workspace/build/src/obj-firefox/dist/include/nsBaseHashtable.h:142:10
    #6 0x7ff19ebe59d9 in mozilla::a11y::DocManager::CreateDocOrRootAccessible(nsIDocument*) /home/worker/workspace/build/src/accessible/base/DocManager.cpp:484:3
    #7 0x7ff19ebe595f in mozilla::a11y::DocManager::CreateDocOrRootAccessible(nsIDocument*) /home/worker/workspace/build/src/accessible/base/DocManager.cpp:470:20
    #8 0x7ff19ebf9760 in mozilla::a11y::SelectionManager::NotifySelectionChanged(nsIDOMDocument*, nsISelection*, short) /home/worker/workspace/build/src/accessible/base/SelectionManager.cpp:175:29
    #9 0x7ff19e299f05 in mozilla::dom::Selection::NotifySelectionListeners() /home/worker/workspace/build/src/layout/generic/nsSelection.cpp:6274:5
    #10 0x7ff19e2a58e4 in mozilla::dom::Selection::RemoveRange(nsRange&, mozilla::ErrorResult&) /home/worker/workspace/build/src/layout/generic/nsSelection.cpp:5111:8
    #11 0x7ff19edc4cf9 in mozInlineSpellChecker::RemoveRange(mozilla::dom::Selection*, nsRange*) /home/worker/workspace/build/src/extensions/spellcheck/src/mozInlineSpellChecker.cpp:1766:3
    #12 0x7ff19edc4664 in mozInlineSpellChecker::DoSpellCheck(mozInlineSpellWordUtil&, mozilla::dom::Selection*, mozInlineSpellStatus*, bool*) /home/worker/workspace/build/src/extensions/spellcheck/src/mozInlineSpellChecker.cpp:1532:11
    #13 0x7ff19edc536a in mozInlineSpellChecker::ResumeCheck(mozInlineSpellStatus*) /home/worker/workspace/build/src/extensions/spellcheck/src/mozInlineSpellChecker.cpp:1682:10
    #14 0x7ff19edc723b in mozInlineSpellResume::Run() /home/worker/workspace/build/src/extensions/spellcheck/src/mozInlineSpellChecker.cpp:492:7
    #15 0x7ff198a5eb62 in nsThread::ProcessNextEvent(bool, bool*) /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1216:7
    #16 0x7ff198aeaca0 in NS_ProcessNextEvent(nsIThread*, bool) /home/worker/workspace/build/src/xpcom/glue/nsThreadUtils.cpp:361:10
    #17 0x7ff199596589 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/worker/workspace/build/src/ipc/glue/MessagePump.cpp:96:21
    #18 0x7ff199504287 in MessageLoop::RunInternal() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:232:3
    #19 0x7ff199504119 in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:205:3
    #20 0x7ff19d9b8a9a in nsBaseAppShell::Run() /home/worker/workspace/build/src/widget/nsBaseAppShell.cpp:156:3
    #21 0x7ff19f15930c in nsAppStartup::Run() /home/worker/workspace/build/src/toolkit/components/startup/nsAppStartup.cpp:283:19
    #22 0x7ff19f2770bd in XREMain::XRE_mainRun() /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4488:10
    #23 0x7ff19f278707 in XREMain::XRE_main(int, char**, nsXREAppData const*) /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4621:8
    #24 0x7ff19f2792f2 in XRE_main /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4712:16
    #25 0x4e03e9 in do_main(int, char**, char**, nsIFile*) /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:282:10
    #26 0x4dfac5 in main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:415:16
    #27 0x7ff1b57a582f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291
    #28 0x41c274 in _start (/home/user/workspace/browsers/m-e-1505415248-asan-debug/firefox+0x41c274)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /home/worker/workspace/build/src/clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.8.5/../../../../include/c++/4.8.5/bits/atomic_base.h:496:9 in std::__atomic_base<unsigned int>::load(std::memory_order) const
==36324==ABORTING

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

Eitan can you triage this one?

Comment 2

[Eitan Isaacson [:eeejay]]

Attachment: Don't use control's frame as reference in SelectionManager. r?surkov

Ported the patch from bug 1330739 here.

Comment 3

[Daniel Veditz [:dveditz]]

(In reply to Eitan Isaacson [:eeejay] from comment #2)
> Ported the patch from bug 1330739 here.

Bug 1330739 claims ESR-52 isn't affected because it was a regression from bug 527003 which landed in 53.

Is that regression range wrong, or is this the wrong solution?

Comment 4

[Eitan Isaacson [:eeejay]]

(In reply to Daniel Veditz [:dveditz] from comment #3)
> (In reply to Eitan Isaacson [:eeejay] from comment #2)
> > Ported the patch from bug 1330739 here.
> 
> Bug 1330739 claims ESR-52 isn't affected because it was a regression from
> bug 527003 which landed in 53.
> 
> Is that regression range wrong, or is this the wrong solution?

Good question. I think the test case provided in bug 1330739 was exploiting a change in bug 527003. I'm not sure where the regression was introduced with this test case, but it causes a similar crash.

Comment 5

[alexander :surkov (:asurkov)]

Comment on attachment 8909413 [details] [diff] [review]
Don't use control's frame as reference in SelectionManager. r?surkov

Review of attachment 8909413 [details] [diff] [review]:
-----------------------------------------------------------------

looks an accurate port

Comment 6

[Eitan Isaacson [:eeejay]]

Comment on attachment 8909413 [details] [diff] [review]
Don't use control's frame as reference in SelectionManager. r?surkov

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Potential UAF
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): Low. It has been in release version for 2 months now.
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

If we end up doing a 52.4 RC respin, we might want to take this while we're at it.

Comment 8

[Julien Cristau [:jcristau]]

Comment on attachment 8909413 [details] [diff] [review]
Don't use control's frame as reference in SelectionManager. r?surkov

ok let's take this for a build2 with 1371889

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/d6f78b1349b7

Comment 10

[Al Billings [:abillings - ex-MoCo]]

How does this affect no shipping version of mainline Firefox but it affected ESR52? That does not compute.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Bug 1330739 fixed it for 55+. Per comments 3 & 4 of this bug, bug 1330739 was erroneously seen as not affecting ESR52 at the time.