845148 - Valgrind warning about use after free in SocketAcceptTask

Status: VERIFIED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

==228== Invalid write of size 4
==228==    at 0x5BFC8E4: mozilla::ipc::SocketAcceptTask::Run() (in /system/b2g/libxul.so)
==228==    by 0x5C47FA1: MessageLoop::RunTask(Task*) (in /system/b2g/libxul.so)
==228==    by 0x5C48F9F: MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) (in /system/b2g/libxul.so)
==228==    by 0x5C49017: MessageLoop::DoDelayedWork(base::TimeTicks*) (in /system/b2g/libxul.so)
==228==    by 0x5C5BB7D: base::MessagePumpLibevent::Run(base::MessagePump::Delegate*) (in /system/b2g/libxul.so)
==228==    by 0x5C47F3D: MessageLoop::RunInternal() (in /system/b2g/libxul.so)
==228==    by 0x5C4801D: MessageLoop::Run() (in /system/b2g/libxul.so)
==228==    by 0x5C5104D: base::Thread::ThreadMain() (in /system/b2g/libxul.so)
==228==    by 0x5C5C065: ThreadFunc(void*) (in /system/b2g/libxul.so)
==228==    by 0x483AE17: __thread_entry (in /system/lib/libc.so)
==228==  Address 0xbae2bc8 is 48 bytes inside a block of size 84 free'd
==228==    at 0x48062B0: free (vg_replace_malloc.c:446)
==228==    by 0x60E3DA7: moz_free (in /system/b2g/libxul.so)
==228==    by 0x5BFCAB3: mozilla::ipc::UnixSocketImpl::~UnixSocketImpl() (in /system/b2g/libxul.so)
==228==    by 0x5BFBE79: mozilla::ipc::DeleteInstanceRunnable<mozilla::ipc::UnixSocketImpl>::Run() (in /system/b2g/libxul.so)
==228==    by 0x5C235DF: nsThread::ProcessNextEvent(bool, bool*) (in /system/b2g/libxul.so)
==228==    by 0x5C00A27: NS_ProcessNextEvent_P(nsIThread*, bool) (in /system/b2g/libxul.so)
==228==    by 0x5B05EE3: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (in /system/b2g/libxul.so)
==228==    by 0x5C47F3D: MessageLoop::RunInternal() (in /system/b2g/libxul.so)
==228==    by 0x5C4801D: MessageLoop::Run() (in /system/b2g/libxul.so)
==228==    by 0x5A81337: nsBaseAppShell::Run() (in /system/b2g/libxul.so)
==228==    by 0x59D5A91: nsAppStartup::Run() (in /system/b2g/libxul.so)
==228==    by 0x5381871: XREMain::XRE_mainRun() (in /system/b2g/libxul.so)

Looks like there is a Cancel method on SocketAcceptTask and SocketConnectTask but mImpl is used before checking mCanceled in Run(). Some of these other task classes hold mImpl members and are not cancelable, so I don't know how those will hold up either.

This may be the cause of bug 832385 since the first thing this Run() method does is set a random pointer to null.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to ben turner [:bent] from comment #0)
> ==228==  Address 0xbae2bc8 is 48 bytes inside a block of size 84 free'd

FWIW this is exactly the offset of the first word of mResultVal in IDBRequest.

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

Blocking since this results in random behaviour that could go as far as exploitable crash.

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

The problem is that in UnixSocket.cpp, we keep multiple bare pointers to UnixSocketImpl instances in tasks that run asynchronously on the IO Thread. The implementation pointer can be deleted before the task is run, and we have a good way to check this at the moment.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

NOTE: This patch is against the b2g18 branch.

I haven't had a chance to fully test this patch, but wanted to get it in the bug. It attempts to fully clean up the destruction path for UnixSocketImpls. The basic idea is this:

We introduce two state bits: mShuttingDownMainThread and mShuttingDownIOThread. When CloseSocket is called (on the main thread), we set mShuttingDownMain to true, send an event to the IO thread and start enforcing that no more events are sent from the main thread to that Impl (for completeness, we also null out mImpl). Once the event runs on the IO thread, we clean up our state to stop receiving "data ready" events and set a bit that means that no more Accept calls are re-queued. From this point on, the Impl will be untouched on the IO thread. We then proxy the delete back to the main thread.

It's worth noting that this approach is similar to what happened on trunk, though that uses a lock (though it only locks around the get and set of a boolean, so might as well not) and this approach is a little more paranoid.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Oh, I realized after writing my comment that mShuttingDownMainThread is now redundant with mConsumer being non-null, so I removed it in favor of that.

Comment 6

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

Comment on attachment 718798 [details] [diff] [review]
patch v1

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

This looks great!

::: ipc/unixsocket/UnixSocket.cpp
@@ +419,5 @@
>    SocketAcceptTask(UnixSocketImpl* aImpl) : mCanceled(false), mImpl(aImpl) { }
> +
> +  virtual void Cancel()
> +  {
> +    mCanceled = true;

This should only happen on the IO thread, right? Let's assert (I thought we did already...).

Comment 7

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

Is this a sec-critical rated issue? Could this be triggered by web content?

Comment 8

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

(In reply to Al Billings [:abillings] from comment #7)
> Is this a sec-critical rated issue? Could this be triggered by web content?

Well, on B2G, all one has to do is disable bluetooth from the settings app. I don't know how we rate that kind of thing...

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v2

I'm sorry, I couldn't refrain from fixing Read... This gets most of the way there as far as my limited testing can tell. Still missing is a patch to make sure all callers of SendSocketData come from the main thread.

Comment 10

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Are you moving the changed to BluetoothOppManager to another bug, or was that supposed to be in this one?

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v2.1

The last patch was overeager in removing the calls to StopWatchingFileDescriptor in the "socket failed" case. I assumed that the socket failing would prevent any more events coming in on it, but that is not the case. This adds them back. Testcase: Send a file. Try to open an application. Note no application opens.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix for the OPP manager

The OPP manager was playing fast and loose with threads: calling into the UnixSocket code from a third thread! Properly synchronizing a third thread would have forced us into the land of locks and even more difficult problems than the ones that we're facing in this bug. Fortunately, the third thread isn't even necessary: we can use the IO thread, meaning that all the synchronization is done for us. One note: it'd be really good for someone to double check that canceling a file transfer can't result in us sending data after we call CloseSocket. I didn't have time to check it fully, but there are MOZ_ASSERTs thrown in there.

Comment 13

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 719287 [details] [diff] [review]
Fix for the OPP manager

Bringing in echou on the OPP manager review since he's the expert on that particular subject. :)

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

This patch represents a really scary change right on a deadline. I've been testing sending and receiving files, but I don't have a bluetooth headset to test and I'm sure that I haven't stress tested things like error conditions enough. Can other people help out here?

Comment 15

[Michael Vines [:m1] [:evilmachines]]

(fwiw: if this lands 2/29 I honestly doubt anybody outside of moz will notice, except for maybe me and I'm over it)

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

To be clear, the result of this bug is that turning bluetooth off and on a few times in a row causes the parent process to crash. I'm not going to try to assign a severity to that.

Comment 17

[Jason Smith [:jsmith]]

+Matt

I'll go hunt for a QA owner.

Comment 19

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

Comment on attachment 719286 [details] [diff] [review]
patch v2.1

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

r=me.

I'm running this through valgrind as we speak.

::: ipc/unixsocket/UnixSocket.cpp
@@ +406,5 @@
> +  SocketAcceptTask(UnixSocketImpl* aImpl) : mImpl(aImpl) { }
> +
> +  virtual void Cancel()
> +  {
> +    mImpl = nullptr;

MOZ_ASSERT(!NS_IsMainThread()) here since this is only ever supposed to be called on IO thread.

@@ +675,2 @@
>          }
> +        else if (errno == EAGAIN || errno == EWOULDBLOCK) {

Nit: else-after-continue :)

@@ +687,5 @@
> +      // cause us to end up back here.
> +      mReadWatcher.StopWatchingFileDescriptor();
> +      mWriteWatcher.StopWatchingFileDescriptor();
> +
> +      nsRefPtr<SocketCloseTask> t = new SocketCloseTask(this);

SocketCloseTask assumes that it has a mConsumer... Do you need to check that? You added a |if (!mConsumer)| check elsewhere here...

Comment 20

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

Comment on attachment 719287 [details] [diff] [review]
Fix for the OPP manager

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

::: ipc/unixsocket/UnixSocket.cpp
@@ +611,5 @@
> +{
> +  MOZ_ASSERT(!NS_IsMainThread());
> +  MOZ_ASSERT(mImpl && !mImpl->IsShutdownOnIOThread());
> +
> +  mImpl->QueueWriteData(aData);

I don't know how we can guarantee that this is safe. Isn't mImpl set (and unset) on the main thread?

Comment 21

[Michael Vines [:m1] [:evilmachines]]

(Greg to cc: since he's tracking bug 845990 from over here)

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 719287 [details] [diff] [review]
Fix for the OPP manager

bent and I have a plan for how to fix this.

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to ben turner [:bent] from comment #19)\
> SocketCloseTask assumes that it has a mConsumer... Do you need to check
> that? You added a |if (!mConsumer)| check elsewhere here...

It has a check in the !IsShutdownOnMainThread call.

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v2.2

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: OPP manager patch v2

This no longer races to get mImpl from the OppManager. It also no longer races to set mLastCommand off the main thread.

The relationship between the classes here makes hacking the code here really weird.

Comment 26

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

Comment on attachment 719694 [details] [diff] [review]
OPP manager patch v2

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

::: dom/bluetooth/BluetoothOppManager.cpp
@@ +124,5 @@
>  public:
> +  ReadFileTask(UnixSocketImpl* aImpl,
> +               nsIInputStream* aInputStream,
> +               uint32_t aRemoteMaxPacketSize)
> +    : mImpl(aImpl),

Assert mImpl

@@ +997,5 @@
> +  // responses come back from the socket.
> +  nsRefPtr<SetLastCommandRunnable> runnable(
> +      new SetLastCommandRunnable(ObexRequestCode::Put));
> +  nsresult rv = NS_DispatchToMainThread(runnable);
> +  NS_ENSURE_SUCCESS_VOID(rv);

This will leak req, let's put that in an autoptr

::: dom/bluetooth/BluetoothOppManager.h
@@ +73,5 @@
>    bool ExtractBlobHeaders();
>  
> +  void SetLastCommand(int aCommand)
> +  {
> +    mLastCommand = aCommand;

This needs a main thread assertion, maybe just move to the cpp.

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/99ff68d01301
https://hg.mozilla.org/releases/mozilla-b2g18/rev/2c40d7d5236a

Please correct me if I'm setting the flags wrong.

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

This needs to be checked into Mozilla-B2g18_v1_0_1 only (I think!)

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/656066ad44e6
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/3b499b6b21e6

Leaving open for the m-c fix to land.

Comment 30

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

Oh, let's clone and have a new one for trunk after we figure out what's going on in bug 830290.

Comment 31

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

This was a branch-only fix. Let's track bug 846615 for anything that needs to happen on trunk (starting with me actually verifying that there is a real problem on trunk).

Comment 32

[John Hammink]

with one other tester, we ran a quick testpass against 03-01 nightly:

Gecko  http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/69e249bf251c
Gaia   f46906e594b613571bfcc8f146d60bffd42e5a5b
BuildID 20130301070202
Version 18.0

partial results of the testrun are here: https://moztrap.mozilla.org/runtests/run/898/env/305/

summary:

accepting phonecall via bt headset - OK
rejecting phonecall via bt headset  - OK
pairing bt device - ok
disconnecting bt device - OK
searching for devices when BT switch on  - OK

however we saw issues where bt file tranfer.
1.) in one case FFOS on the device crashed while trying to send a file from unagi (happened once);
2.) not able to establish connection between macbook air and unagi device to send a file from unagi to laptop (laptop says "failed to establish connection')

3.) in some cases though, after a repair, we can send files to laptop to unagi
 Note that we'll be investigating this more when we have more time but this is the case for now.

Comment 33

[Jason Smith [:jsmith]]

I'll wait for the followup bugs to be filed for the items in comment 32, then we can remove qawanted on the bug.

Comment 34

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

John, have you written up the follow up bugs for this yet?  If so, can you please list them?

Comment 35

[John Hammink]

Hi,

Here are a few bugs currently related to this issue:

https://bugzilla.mozilla.org/show_bug.cgi?id=847963 -  sometimes cannot transfer files to a mac

https://bugzilla.mozilla.org/show_bug.cgi?id=823803 -  [Open_][bluetooth] Bluetooth file which The test machine to accept not be able to view, when accept completed-

To be clear, I want to try and test the above scenario on the latest nightly, to see where things are.

Comment 36

[John Hammink]

Tested BT File transfer, and and BT Headset (answering call) on the following builds.

accepting phonecall via bt headset - OK
terminating phonecall via bt headset - OK
pairing bt device - ok
disconnecting bt device - OK
searching for devices when BT switch on  - OK


Tested on both 1.0.1. and 1.1 channels.
Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/99680a32f297
Gaia   de3e5b9205e6cb1a6bd0858a98d159272ad96d11
BuildID 20130312070202
Version 18.0


Also tested on
Gecko  http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/cdf058e47197
Gaia   1a6e1cd7715a5192b32db4b7127c5ae5b8162a7a
BuildID 20130312070203
Version 18.0

Comment 37

[Blake Kaplan (:mrbkap) (inactive)]

Thanks John.

Comment 38

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 719694 [details] [diff] [review]
OPP manager patch v2

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

Remove review request since it has been resolved.