1452202 - Undefined behavior in PLDHashTable::operator=()

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[mozillabugs]

PLDHashTable::operator=() (xpcom/ds/PLDHashTable.cpp) invokes undefined behavior by destroying |this|, then using the destructed object's nonstatic member variables. This directly violates C++11 s.3.8, which says:

------
The lifetime of an object of type T ends when:
— if T is a class type with a non-trivial destructor (12.4), the destructor call starts...

[A]fter the lifetime of an object has ended and before the storage which the object occupied is reused or released, any pointer that refers to the storage location where the object will be or was located may be used but only in limited ways. For an object under construction or destruction, see 12.7. Otherwise, such a pointer refers to allocated storage (3.7.4.2), and using the pointer as if the pointer were of type void*, is well-defined. Such a pointer may be dereferenced but the resulting lvalue may only be used in limited ways, as described below. 

**** The program has undefined behavior if:...
— the pointer is used to access a non-static data member or call a non-static member function of the object.... ****
------

(emphasis added).

The bug is on lines 235, 236, 239, et seq., and on every other non-static member variable reference thereafter made to the destination object (and also the delete call that ultimately destroys it):

220: PLDHashTable&
221: PLDHashTable::operator=(PLDHashTable&& aOther)
222: {
223:   if (this == &aOther) {
224:     return *this;
225:   }
226: 
227:   // Destruct |this|.
228:   this->~PLDHashTable();
229: 
230:   // |mOps| and |mEntrySize| are const so we can't assign them. Instead, we
231:   // require that they are equal. The justification for this is that they're
232:   // conceptually part of the type -- indeed, if PLDHashTable was a templated
233:   // type like nsTHashtable, they *would* be part of the type -- so it only
234:   // makes sense to assign in cases where they match.
235:   MOZ_RELEASE_ASSERT(mOps == aOther.mOps);
236:   MOZ_RELEASE_ASSERT(mEntrySize == aOther.mEntrySize);
237: 
238:   // Move non-const pieces over.
239:   mHashShift = Move(aOther.mHashShift);
240:   mEntryCount = Move(aOther.mEntryCount);
241:   mRemovedCount = Move(aOther.mRemovedCount);
242:   mEntryStore.Set(aOther.mEntryStore.Get(), &mGeneration);
243: #ifdef DEBUG
244:   mChecker = Move(aOther.mChecker);
245: #endif
246: 
247:   // Clear up |aOther| so its destruction will be a no-op.
248:   {
249: #ifdef DEBUG
250:     AutoDestructorOp op(mChecker);
251: #endif
252:     aOther.mEntryStore.Set(nullptr, &aOther.mGeneration);
253:   }
254: 
255:   return *this;
256: }

This code is actually used (with a nonidentical source object), though DXR says that it isn't [1]. I found one usage by attaching a debugger to FF and loading https://www.washingtonpost.com , with the stack:

>	xul.dll!PLDHashTable::operator=(PLDHashTable && aOther) Line 228	C++
 	xul.dll!PLDHashTable::PLDHashTable(PLDHashTable && aOther) Line 301	C++
 	xul.dll!mozilla::Swap<PLDHashTable>(PLDHashTable & aX, PLDHashTable & aY) Line 232	C++
 	xul.dll!nsTHashtable<detail::VoidPtrHashKey>::SwapElements(nsTHashtable<detail::VoidPtrHashKey> & aOther) Line 331	C++
 	xul.dll!nsTHashtable<nsPtrHashKey<nsIFrame> >::SwapElements(nsTHashtable<nsPtrHashKey<nsIFrame> > & aOther) Line 623	C++
 	xul.dll!mozilla::PresShell::RebuildApproximateFrameVisibility(nsRect * aRect, bool aRemoveOnly) Line 6069	C++
 	xul.dll!mozilla::PresShell::DoUpdateApproximateFrameVisibility(bool aRemoveOnly) Line 6112	C++
 	xul.dll!mozilla::PresShell::UpdateApproximateFrameVisibility() Line 6090	C++
 	xul.dll!mozilla::detail::RunnableMethodArguments<>::applyImpl<mozilla::PresShell,void (__cdecl mozilla::PresShell::*)(void) __ptr64>(mozilla::PresShell * o, void(mozilla::PresShell::*)() m, mozilla::Tuple<> & args, mozilla::IndexSequence<> __formal) Line 1143	C++
 	xul.dll!mozilla::detail::RunnableMethodArguments<>::apply<mozilla::PresShell,void (__cdecl mozilla::PresShell::*)(void) __ptr64>(mozilla::PresShell * o, void(mozilla::PresShell::*)() m) Line 1150	C++
 	xul.dll!mozilla::detail::RunnableMethodImpl<mozilla::PresShell * __ptr64,void (__cdecl mozilla::PresShell::*)(void) __ptr64,1,0>::Run() Line 1195	C++
 	xul.dll!mozilla::SchedulerGroup::Runnable::Run() Line 396	C++
 	xul.dll!nsThread::ProcessNextEvent(bool aMayWait, bool * aResult) Line 1038	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * aThread, bool aMayWait) Line 513	C++
 	xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate) Line 97	C++
 	xul.dll!mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate * aDelegate) Line 302	C++
 	xul.dll!MessageLoop::RunInternal() Line 327	C++
 	xul.dll!MessageLoop::RunHandler() Line 320	C++
 	xul.dll!MessageLoop::Run() Line 300	C++
 	xul.dll!nsBaseAppShell::Run() Line 161	C++
 	xul.dll!nsAppShell::Run() Line 230	C++
 	xul.dll!XRE_RunAppShell() Line 877	C++
 	xul.dll!mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate * aDelegate) Line 269	C++
 	xul.dll!MessageLoop::RunInternal() Line 327	C++
 	xul.dll!MessageLoop::RunHandler() Line 320	C++
 	xul.dll!MessageLoop::Run() Line 300	C++
 	xul.dll!XRE_InitChildProcess(int aArgc, char * * aArgv, const XREChildData * aChildData) Line 707	C++
 	xul.dll!mozilla::BootstrapImpl::XRE_InitChildProcess(int argc, char * * argv, const XREChildData * aChildData) Line 70	C++
 	firefox.exe!content_process_main(mozilla::Bootstrap * bootstrap, int argc, char * * argv) Line 63	C++
 	firefox.exe!NS_internal_main(int argc, char * * argv, char * * envp) Line 280	C++
 	firefox.exe!wmain(int argc, wchar_t * * argv) Line 111	C++
 	[External Code]	

Maybe this bug is behind some of the numerous PLDHashTable-related crashes that have been reported? https://bugzilla.mozilla.org/buglist.cgi?quicksearch=PLDHashTable .

[1] DXR indicates that operator=(&&) is called only by the PLDHashTable's move contructor, which DXR then says is used only by xpcom/tests/gtest/TestPLDHash.cpp . Alas, DXR sometimes misses uses.

Comment 1

[Eric Rahm [:erahm]]

Attachment: Clean up PLDHashTable move operator

Comment 2

[Eric Rahm [:erahm]]

Comment on attachment 8966300 [details] [diff] [review]
Clean up PLDHashTable move operator

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

::: xpcom/ds/PLDHashTable.cpp
@@ +228,2 @@
>    this->~PLDHashTable();
> +  new (KnownNotNull, this) PLDHashTable(aOther.mOps, aOther.mEntrySize, 0);

So if we reconstruct the object |this| should be okay again and we get rid of the undefined behavior. I think 0 is fine for the length here (it's just temporary), but I haven't tested it.

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8966300 [details] [diff] [review]
Clean up PLDHashTable move operator

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

Thanks for picking this up.  We should implement a specialization of Swap on PLDHashTable for some followup bug.

::: xpcom/ds/PLDHashTable.cpp
@@ -232,5 @@
> -  // conceptually part of the type -- indeed, if PLDHashTable was a templated
> -  // type like nsTHashtable, they *would* be part of the type -- so it only
> -  // makes sense to assign in cases where they match.
> -  MOZ_RELEASE_ASSERT(mOps == aOther.mOps);
> -  MOZ_RELEASE_ASSERT(mEntrySize == aOther.mEntrySize);

Why are we deleting these?  If anything, we should be pulling out mOps and mEntrySize before calling the destructor--or moving the assertions prior to the destructor.

Comment 4

[Eric Rahm [:erahm]]

(In reply to Nathan Froyd [:froydnj] from comment #3)
> Comment on attachment 8966300 [details] [diff] [review]
> Clean up PLDHashTable move operator
> 
> Review of attachment 8966300 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for picking this up.  We should implement a specialization of Swap on
> PLDHashTable for some followup bug.
> 
> ::: xpcom/ds/PLDHashTable.cpp
> @@ -232,5 @@
> > -  // conceptually part of the type -- indeed, if PLDHashTable was a templated
> > -  // type like nsTHashtable, they *would* be part of the type -- so it only
> > -  // makes sense to assign in cases where they match.
> > -  MOZ_RELEASE_ASSERT(mOps == aOther.mOps);
> > -  MOZ_RELEASE_ASSERT(mEntrySize == aOther.mEntrySize);
> 
> Why are we deleting these?  If anything, we should be pulling out mOps and
> mEntrySize before calling the destructor--or moving the assertions prior to
> the destructor.

They were there because the fields are const and can only be assigned in the constructor, so instead we just asserted they didn't change. Now that we're using the constructor we don't have that limitation. I can add them back if you want though.

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #4)
> (In reply to Nathan Froyd [:froydnj] from comment #3)
> > ::: xpcom/ds/PLDHashTable.cpp
> > @@ -232,5 @@
> > > -  // conceptually part of the type -- indeed, if PLDHashTable was a templated
> > > -  // type like nsTHashtable, they *would* be part of the type -- so it only
> > > -  // makes sense to assign in cases where they match.
> > > -  MOZ_RELEASE_ASSERT(mOps == aOther.mOps);
> > > -  MOZ_RELEASE_ASSERT(mEntrySize == aOther.mEntrySize);
> > 
> > Why are we deleting these?  If anything, we should be pulling out mOps and
> > mEntrySize before calling the destructor--or moving the assertions prior to
> > the destructor.
> 
> They were there because the fields are const and can only be assigned in the
> constructor, so instead we just asserted they didn't change. Now that we're
> using the constructor we don't have that limitation.

Sure, but after the current proposed change, you could do something like:

// some ops != other ops
PLDHashTable ht1(some ops, some entry size, ...);
PLDHashTable ht2(other ops, other entry size, ...);
ht1 = Move(ht2);

and be just fine, whereas before, the asserts would have complained, loudly.  const-ness was a problem before, but we were also checking that the tables were compatible, in some sense, to be used in assignment.  That check is still valuable for catching mistakes.

Comment 6

[Eric Rahm [:erahm]]

Attachment: Clean up PLDHashTable move operator

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8966326 [details] [diff] [review]
Clean up PLDHashTable move operator

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

Perfect, thank you.

Comment 8

[Eric Rahm [:erahm]]

Comment on attachment 8966326 [details] [diff] [review]
Clean up PLDHashTable move operator

This is an unrated sec bug related to undefined behavior possibly leading to UAF or more likely crashes due to accessing an invalid |this| pointer.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Seems like it would be difficult, we're looking at undefined behavior during a somewhat rare move operation.

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?

All.

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

All, looks like this came from bug 1170934 in 2015.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

It should apply cleanly.

How likely is this patch to cause regressions; how much testing does it need?

Low likelyhood, it's just cleaning up undefined behavior and is essentially a one line change.

Comment 9

[Daniel Veditz [:dveditz]]

> Maybe this bug is behind some of the numerous PLDHashTable-related crashes
> that have been reported? https://bugzilla.mozilla.org/buglist.cgi?quicksearch=PLDHashTable

Most of the hashtable crashes are because the lifetime of the objects in the hashtable are screwed up by their use outside the hashtable. Later on it dies in hashtable code when it tries to operate on the now-gone object. They're often hard to figure out because the crime was nowhere near where the body was found.

Comment 10

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

Clearing sec-approval since Dan made this a sec-moderate rated issue. This can just land on trunk and ride the trains. If you want to make an argument for Beta and ESR52, we'd certainly entertain the idea of taking it there.

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9036c64b7a66ffe93e717ca97642a4400e396d9c
https://hg.mozilla.org/mozilla-central/rev/9036c64b7a66

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Given what a tiny patch it is, there's enough PLDHashTable crashes on ESR52 that if this can help with them at all, it seems worth taking. Please nominate for Beta and ESR52 uplift unless you feel strongly otherwise, Eric :). It grafts cleanly to both.

Comment 13

[Eric Rahm [:erahm]]

Comment on attachment 8966326 [details] [diff] [review]
Clean up PLDHashTable move operator

Approval Request Comment
[Feature/Bug causing the regression]: bug 1170934
[User impact if declined]: Possible crashes, UAF in hash table code.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Landed in nightly.
[Needs manual test from QE? If yes, steps to reproduce]:  No.
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: Low risk
[Why is the change risky/not risky?]: essentially one line of code that properly initializes an obejct
[String changes made/needed]: N/A

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: We think it might help with PLDHashTable crashes.
Fix Landed on Version: 61
Risk to taking this patch (and alternatives if risky): Low risk, essentially one line change the properly initializes an object.
String or UUID changes made by this patch: N/A

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8966326 [details] [diff] [review]
Clean up PLDHashTable move operator

Simple fix that may help with the various PLDHashTable crashes we see. Approved for 60.0b13 and ESR 52.8.0.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/041d1c561feb

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/134c728799c1

Comment 17

[Andrei Vaida [:avaida]]

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #13)
> [Is this code covered by automated tests?]: Yes.
> [Needs manual test from QE? If yes, steps to reproduce]:  No.
> [Is the change risky?]: Low risk

Does not need manual testing, per Eric.