844422 - Valgrind warning about System Messages being used after free on b2g18

Status: RESOLVED WORKSFORME

(Firefox OS Graveyard :: General, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

==231== Invalid write of size 4
==231==    at 0x5AE679C: mozilla::dom::ContentParent::RecvSystemMessageHandled() (in /system/b2g/libxul.so)
==231==    by 0x5B383CB: mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) (in /system/b2g/libxul.so)
==231==    by 0x5B025CB: mozilla::ipc::AsyncChannel::OnDispatchMessage(IPC::Message const&) (in /system/b2g/libxul.so)
==231==    by 0x5B07B85: mozilla::ipc::RPCChannel::OnMaybeDequeueOne() (in /system/b2g/libxul.so)
==231==    by 0x5AE668F: RunnableMethod<mozilla::dom::TabChild, void (mozilla::dom::TabChild::*)(), Tuple0>::Run() (in /system/b2g/libxul.so)
==231==    by 0x5B063E1: mozilla::ipc::RPCChannel::DequeueTask::Run() (in /system/b2g/libxul.so)
==231==    by 0x5C47EF1: MessageLoop::RunTask(Task*) (in /system/b2g/libxul.so)
==231==    by 0x5C48EEF: MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) (in /system/b2g/libxul.so)
==231==    by 0x5C49BB1: MessageLoop::DoWork() (in /system/b2g/libxul.so)
==231==    by 0x5B05D19: mozilla::ipc::DoWorkRunnable::Run() (in /system/b2g/libxul.so)
==231==    by 0x5C2352F: nsThread::ProcessNextEvent(bool, bool*) (in /system/b2g/libxul.so)
==231==    by 0x5C00977: NS_ProcessNextEvent_P(nsIThread*, bool) (in /system/b2g/libxul.so)
==231==  Address 0xdd26218 is 8 bytes inside a block of size 28 free'd
==231==    at 0x48062B0: free (vg_replace_malloc.c:446)
==231==    by 0x60E4247: moz_free (in /system/b2g/libxul.so)
==231==    by 0x5AE6965: mozilla::dom::(anonymous namespace)::SystemMessageHandledListener::Release() (in /system/b2g/libxul.so)
==231==    by 0x53D0665: nsRefPtr<imgRequest>::assign_assuming_AddRef(imgRequest*) (in /system/b2g/libxul.so)
==231==    by 0x579E335: nsCOMPtr_base::assign_with_AddRef(nsISupports*) (in /system/b2g/libxul.so)
==231==    by 0x5C25609: nsTimerImpl::Fire() (in /system/b2g/libxul.so)
==231==    by 0x5C2567F: nsTimerEvent::Run() (in /system/b2g/libxul.so)
==231==    by 0x5C2352F: nsThread::ProcessNextEvent(bool, bool*) (in /system/b2g/libxul.so)
==231==    by 0x5C00977: NS_ProcessNextEvent_P(nsIThread*, bool) (in /system/b2g/libxul.so)
==231==    by 0x5B05E3B: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (in /system/b2g/libxul.so)
==231==    by 0x5C47E8D: MessageLoop::RunInternal() (in /system/b2g/libxul.so)
==231==    by 0x5C47F6D: MessageLoop::Run() (in /system/b2g/libxul.so)

This is an optimized build... RecvSystemMessageHandled calls SystemMessageHandledListener::OnSystemMessageHandled() so I think the bad part is somewhere in here:

    class SystemMessageHandledListener
    {
    ...
        static void OnSystemMessageHandled()
        {
            if (!sListeners) {
                return;
            }

            SystemMessageHandledListener* listener = sListeners->popFirst();
            if (!listener) {
                return;
            }

            // Careful: ShutDown() may delete |this|.
            listener->ShutDown();
        }
    ...
    };

It may be that sListeners holds SystemMessageHandledListener objects that have been deleted.

I triggered this by using the notification panel and repeatedly enabling/disabling bluetooth, wifi, and airplane mode in a somewhat random fashion.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Maybe this is somehow related to WakeLock's:

==231== Invalid read of size 1
==231==    at 0x5765506: mozilla::dom::power::WakeLock::Unlock() (in /system/b2g/libxul.so)
==231==    by 0x5AE6755: mozilla::dom::(anonymous namespace)::SystemMessageHandledListener::ShutDown() (in /system/b2g/libxul.so)
==231==    by 0x5AE67A1: mozilla::dom::ContentParent::RecvSystemMessageHandled() (in /system/b2g/libxul.so)
==231==    by 0x5B383CB: mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) (in /system/b2g/libxul.so)
==231==    by 0x5B025CB: mozilla::ipc::AsyncChannel::OnDispatchMessage(IPC::Message const&) (in /system/b2g/libxul.so)
==231==    by 0x5B07B85: mozilla::ipc::RPCChannel::OnMaybeDequeueOne() (in /system/b2g/libxul.so)
==231==    by 0x5AE668F: RunnableMethod<mozilla::dom::TabChild, void (mozilla::dom::TabChild::*)(), Tuple0>::Run() (in /system/b2g/libxul.so)
==231==    by 0x5B063E1: mozilla::ipc::RPCChannel::DequeueTask::Run() (in /system/b2g/libxul.so)
==231==    by 0x5C47EF1: MessageLoop::RunTask(Task*) (in /system/b2g/libxul.so)
==231==    by 0x5C48EEF: MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) (in /system/b2g/libxul.so)
==231==    by 0x5C49BB1: MessageLoop::DoWork() (in /system/b2g/libxul.so)
==231==    by 0x5B05D19: mozilla::ipc::DoWorkRunnable::Run() (in /system/b2g/libxul.so)
==231==  Address 0xdd24050 is 24 bytes inside a block of size 56 free'd
==231==    at 0x48062B0: free (vg_replace_malloc.c:446)
==231==    by 0x60E4247: moz_free (in /system/b2g/libxul.so)
==231==    by 0x57657CB: mozilla::dom::power::WakeLock::~WakeLock() (in /system/b2g/libxul.so)
==231==    by 0x55CECE9: nsDOMParser::Release() (in /system/b2g/libxul.so)
==231==    by 0x538029F: nsCollation::~nsCollation() (in /system/b2g/libxul.so)
==231==    by 0x5AE695F: mozilla::dom::(anonymous namespace)::SystemMessageHandledListener::Release() (in /system/b2g/libxul.so)
==231==    by 0x53D0665: nsRefPtr<imgRequest>::assign_assuming_AddRef(imgRequest*) (in /system/b2g/libxul.so)
==231==    by 0x579E335: nsCOMPtr_base::assign_with_AddRef(nsISupports*) (in /system/b2g/libxul.so)
==231==    by 0x5C25609: nsTimerImpl::Fire() (in /system/b2g/libxul.so)
==231==    by 0x5C2567F: nsTimerEvent::Run() (in /system/b2g/libxul.so)
==231==    by 0x5C2352F: nsThread::ProcessNextEvent(bool, bool*) (in /system/b2g/libxul.so)
==231==    by 0x5C00977: NS_ProcessNextEvent_P(nsIThread*, bool) (in /system/b2g/libxul.so)

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I'm pretty sure this will end up calling a virtual function on deleted memory (Shutdown() calls AddRef() immediately). s-s.

Comment 3

[Alex Keybl [:akeybl]]

(In reply to ben turner [:bent] from comment #2)
> I'm pretty sure this will end up calling a virtual function on deleted
> memory (Shutdown() calls AddRef() immediately). s-s.

So would we call this an sec-critical bug? Outside of security, what's the user impact here?

Comment 4

[Justin Lebar (not reading bugmail)]

bent and I talked about this on irc.  Given that he's not seeing this on m-c, I'm pretty sure the issue is caused by the fact that bug 803688 is not on b2g18.

We can add a remove() call to the destructor here, or backport the fix.

Comment 5

[Justin Lebar (not reading bugmail)]

I'd rather backport bug 803688, so that we have consistent semantics across our development branches.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Alex Keybl [:akeybl] from comment #3)
> So would we call this an sec-critical bug?

I'm out of the loop here, better to have a security team member answer that.

> Outside of security, what's the user impact here?

Crashes! Potentially any time we handle a system message (receive a call, get an sms, etc.).

Comment 7

[Justin Lebar (not reading bugmail)]

This can cause exploitable crashes whenever we receive a call or SMS.  We have a simple, safe fix in bug 803688.

Please a+ that patch ASAP.

Comment 8

[Alex Keybl [:akeybl]]

Thanks all, we'll take bug 803688 in that case and not tef+ this bug specifically.

Comment 9

[Daniel Veditz [:dveditz]]

This should be fixed now if bug 803688 did the trick on the b2g18 branch. Can you confirm and close the bug, Ben?

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Yep, my testing show that this looks good now. Thanks Justin!