Create temp refs on proxies.
Fixes the following scenario:
1) Thread T1 receives a binder proxy and adds a local strong/weak ref
2) Thread T1 queues BC_ACQUIRE/BC_INCREFS (but does not flush)
3) Thread T1 hands the binder proxy off to another thread T2, which
adds another strong ref
4) Thread T1 now drops its own ref to the proxy, but it doesn't get
destructed because T2 has its own ref to it
6) Thread T2 runs and drops its own ref to the proxy, which causes a
BC_RELEASE/BC_DECREFS to be queued to the driver.
Now, if T1 writes its command queue to the driver first, everything
is fine, because a BC_ACQUIRE/BC_INCREFS will be followed by a
BC_RELEASE/BC_DECREFS. However if T2 writes its command queue first,
BC_RELEASE/BC_DECREFS will be sent first, and the driver will delete
the reference prematurely.
Fix this by temporarily holding a weak/strong ref until the initial
BC_ACUIRE/BC_INCREFS is flushed to the driver.
Bug: 78437964
Test: sailfish builds, boots, no invalid ref messages on dumpsys
Change-Id: I127b85e12a415a1a1ca6e7082e2049513a347f9b
diff --git a/BpHwBinder.cpp b/BpHwBinder.cpp
index ed7ed2a..90b3c6f 100644
--- a/BpHwBinder.cpp
+++ b/BpHwBinder.cpp
@@ -96,7 +96,7 @@
ALOGV("Creating BpHwBinder %p handle %d\n", this, mHandle);
extendObjectLifetime(OBJECT_LIFETIME_WEAK);
- IPCThreadState::self()->incWeakHandle(handle);
+ IPCThreadState::self()->incWeakHandle(handle, this);
}
status_t BpHwBinder::transact(
@@ -282,7 +282,7 @@
{
ALOGV("onFirstRef BpHwBinder %p handle %d\n", this, mHandle);
IPCThreadState* ipc = IPCThreadState::self();
- if (ipc) ipc->incStrongHandle(mHandle);
+ if (ipc) ipc->incStrongHandle(mHandle, this);
}
void BpHwBinder::onLastStrongRef(const void* /*id*/)
diff --git a/IPCThreadState.cpp b/IPCThreadState.cpp
index a1d448d..d29ae51 100644
--- a/IPCThreadState.cpp
+++ b/IPCThreadState.cpp
@@ -403,6 +403,15 @@
if (mProcess->mDriverFD <= 0)
return;
talkWithDriver(false);
+ // The flush could have caused post-write refcount decrements to have
+ // been executed, which in turn could result in BC_RELEASE/BC_DECREFS
+ // being queued in mOut. So flush again, if we need to.
+ if (mOut.dataSize() > 0) {
+ talkWithDriver(false);
+ }
+ if (mOut.dataSize() > 0) {
+ ALOGW("mOut.dataSize() > 0 after flushCommands()");
+ }
}
void IPCThreadState::blockUntilThreadAvailable()
@@ -498,6 +507,27 @@
}
}
+void IPCThreadState::processPostWriteDerefs()
+{
+ /*
+ * libhwbinder has a flushCommands() in the BpHwBinder destructor,
+ * which makes this function (potentially) reentrant.
+ * New entries shouldn't be added though, so just iterating until empty
+ * should be safe.
+ */
+ while (mPostWriteWeakDerefs.size() > 0) {
+ RefBase::weakref_type* refs = mPostWriteWeakDerefs[0];
+ mPostWriteWeakDerefs.removeAt(0);
+ refs->decWeak(mProcess.get());
+ }
+
+ while (mPostWriteStrongDerefs.size() > 0) {
+ RefBase* obj = mPostWriteStrongDerefs[0];
+ mPostWriteStrongDerefs.removeAt(0);
+ obj->decStrong(mProcess.get());
+ }
+}
+
void IPCThreadState::joinThreadPool(bool isMain)
{
LOG_THREADPOOL("**** THREAD %p (PID %d) IS JOINING THE THREAD POOL\n", (void*)pthread_self(), getpid());
@@ -630,11 +660,14 @@
return err;
}
-void IPCThreadState::incStrongHandle(int32_t handle)
+void IPCThreadState::incStrongHandle(int32_t handle, BpHwBinder *proxy)
{
LOG_REMOTEREFS("IPCThreadState::incStrongHandle(%d)\n", handle);
mOut.writeInt32(BC_ACQUIRE);
mOut.writeInt32(handle);
+ // Create a temp reference until the driver has handled this command.
+ proxy->incStrong(mProcess.get());
+ mPostWriteStrongDerefs.push(proxy);
}
void IPCThreadState::decStrongHandle(int32_t handle)
@@ -644,11 +677,14 @@
mOut.writeInt32(handle);
}
-void IPCThreadState::incWeakHandle(int32_t handle)
+void IPCThreadState::incWeakHandle(int32_t handle, BpHwBinder *proxy)
{
LOG_REMOTEREFS("IPCThreadState::incWeakHandle(%d)\n", handle);
mOut.writeInt32(BC_INCREFS);
mOut.writeInt32(handle);
+ // Create a temp reference until the driver has handled this command.
+ proxy->getWeakRefs()->incWeak(mProcess.get());
+ mPostWriteWeakDerefs.push(proxy->getWeakRefs());
}
void IPCThreadState::decWeakHandle(int32_t handle)
@@ -904,8 +940,10 @@
if (bwr.write_consumed > 0) {
if (bwr.write_consumed < mOut.dataSize())
mOut.remove(0, bwr.write_consumed);
- else
+ else {
mOut.setDataSize(0);
+ processPostWriteDerefs();
+ }
}
if (bwr.read_consumed > 0) {
mIn.setDataSize(bwr.read_consumed);
diff --git a/include/hwbinder/IPCThreadState.h b/include/hwbinder/IPCThreadState.h
index 43cd6f7..bed592b 100644
--- a/include/hwbinder/IPCThreadState.h
+++ b/include/hwbinder/IPCThreadState.h
@@ -65,9 +65,9 @@
uint32_t code, const Parcel& data,
Parcel* reply, uint32_t flags);
- void incStrongHandle(int32_t handle);
+ void incStrongHandle(int32_t handle, BpHwBinder *proxy);
void decStrongHandle(int32_t handle);
- void incWeakHandle(int32_t handle);
+ void incWeakHandle(int32_t handle, BpHwBinder *proxy);
void decWeakHandle(int32_t handle);
status_t attemptIncStrongHandle(int32_t handle);
static void expungeHandle(int32_t handle, IBinder* binder);
@@ -112,6 +112,7 @@
status_t getAndExecuteCommand();
status_t executeCommand(int32_t command);
void processPendingDerefs();
+ void processPostWriteDerefs();
void clearCaller();
@@ -125,7 +126,8 @@
const pid_t mMyThreadId;
Vector<BHwBinder*> mPendingStrongDerefs;
Vector<RefBase::weakref_type*> mPendingWeakDerefs;
-
+ Vector<RefBase*> mPostWriteStrongDerefs;
+ Vector<RefBase::weakref_type*> mPostWriteWeakDerefs;
Parcel mIn;
Parcel mOut;
status_t mLastError;