1302241 - Indexing error in EventListenerManager::RemoveEventListenerInternal() might cause use after free

Status: RESOLVED DUPLICATE of bug 1300606

(Core :: DOM: Events, defect)

Description

[q1]

EventListenerManager::RemoveEventListenerInternal() (dom\events\EventListenerManager.cpp) incorrectly skips the next element after removing a matching event listener object from its |mListeners| array. This can cause it to leave in place an event listener object that it should have removed. Presumably an event might later be dispatched through such an object, causing an unexpected (and likely invalid) call to its event-handling function.

I don't know enough JS to make this occur, but it seems likely that it can occur.

This bug is still present in trunk.

Details:
--------

The bug is in line 622:

590: void
591: EventListenerManager::RemoveEventListenerInternal(
592:                         const EventListenerHolder& aListenerHolder,
593:                         EventMessage aEventMessage,
594:                         nsIAtom* aUserType,
595:                         const nsAString& aTypeString,
596:                         const EventListenerFlags& aFlags,
597:                         bool aAllEvents)
598: {
599:   if (!aListenerHolder || !aEventMessage || mClearingListeners) {
600:     return;
601:   }
602: 
603:   Listener* listener;
604: 
605:   uint32_t count = mListeners.Length();
606:   uint32_t typeCount = 0;
607:   bool deviceType = IsDeviceType(aEventMessage);
608: #ifdef MOZ_B2G
609:   bool timeChangeEvent = (aEventMessage == eTimeChange);
610:   bool networkEvent = (aEventMessage == eNetworkUpload ||
611:                        aEventMessage == eNetworkDownload);
612: #endif // MOZ_B2G
613: 
614:   for (uint32_t i = 0; i < count; ++i) {
615:     listener = &mListeners.ElementAt(i);
616:     if (EVENT_TYPE_EQUALS(listener, aEventMessage, aUserType, aTypeString,
617:                           aAllEvents)) {
618:       ++typeCount;
619:       if (listener->mListener == aListenerHolder &&
620:           listener->mFlags.EqualsIgnoringTrustness(aFlags)) {
621:         RefPtr<EventListenerManager> kungFuDeathGrip(this);
622:         mListeners.RemoveElementAt(i);

...which leaves |i| unchanged. But RemoveElementAt() shifts down all the other elements one index position (e.g., after removing element 2, element 2 contains what used to be element 3). However, when control returns to line 614, |i| gets incremented (e.g., to 3). This means that the next iteration of the loop does not examine the new element at the old index position (e.g., 2).

623:         --count;
624:         mNoListenerForEvent = eVoidEvent;
625:         mNoListenerForEventAtom = nullptr;
626:         if (mTarget && aUserType) {
627:           mTarget->EventListenerRemoved(aUserType);
628:         }
629:         if (mIsMainThreadELM && mTarget) {
630:           EventListenerService::NotifyAboutMainThreadListenerChange(mTarget,
631:                                                                     aUserType);
632:         }
633: 
634:         if (!deviceType
635: #ifdef MOZ_B2G
636:             && !timeChangeEvent && !networkEvent
637: #endif // MOZ_B2G
638:             ) {
639:           return;
640:         }
641:         --typeCount;
642:       }
643:     }
644:   }

https://bugzilla.mozilla.org/show_bug.cgi?id=1294193 might be a related bug.

Comment 1

[Andrew McCreight [:mccr8]]

Olli, could you look at this? This kind of sounds like that issue you filed a bug on recently.

Comment 2

[Andrew McCreight [:mccr8]]

I think this is a dupe of bug 1300606.

Comment 3

[q1]

The patch for https://bugzilla.mozilla.org/show_bug.cgi?id=1300606 doesn't fix this bug, and seems to introduce a new one. See that bug for details.

Comment 4

[q1]

(In reply to q1 from comment #3)
> The patch for https://bugzilla.mozilla.org/show_bug.cgi?id=1300606 doesn't
> fix this bug, and seems to introduce a new one. See that bug for details.

No, that's not correct. It does fix this bug, by introducing a "return" after removing one element.