Android Do not do binder operation in destructor

Description : Do not do binder operation in destructor.

Risk : it’s possible to introduce the binder thread will receive consecutive BR_TRANSACTION_COMPLETE and cause process crash.

// bad example
MediaPlayer::~MediaPlayer()
{
ALOGV("destructor");
if (mAudioAttributesParcel != NULL) {
delete mAudioAttributesParcel;
mAudioAttributesParcel = NULL;
}
// it will issue a binder operations to AudioSystem service
AudioSystem::releaseAudioSessionId(mAudioSessionId, -1);
...

Case Study

Issue Description

system_server crashed because of receiving BAD COMMAND 29190 (29190 means BR_TRANSACTION_COMPLETE, you could refer to ioctl.h or this /article/8573958.html) In normal case, after the binder thread on system_server/AP processes/native processes issues BC_XXXX command, it will wait the binder driver responses a BR_TRANSACTION_COMPLETE to indicate this transaction is done. In this abnormal case, the binder thread issues another BC_XXXX command before receiving the 1st BR_TRANSACATION_COMPLETE. It will cause the binder driver returns two BR_TRANSACTION_COMPLETE consecutively. While binder thread receives the 2nd BR_TRANSACTION_COMPLETE, it thinks this is an invalid command from the binder driver, and invokes abort().

[Error Log]

12-27 10:14:54.272 955 1585 E IPCThreadState: * BAD COMMAND 29190 received from Binder driver

12-27 10:14:54.272 955 1585 E IPCThreadState: getAndExecuteCommand(fd=12) returned unexpected error -2147483648, aborting

12-27 10:14:54.272 955 1585 F libc : Fatal signal 6 (SIGABRT), code -6 in tid 1585 (Binder_D)

Normal Binder Flow (The picture is modified from http://wangkuiwu.github.io/2014/09/05/BinderCommunication-AddService01/)



Abnormal Binder Flow



unexpected BC_XXXX command is issues while the binder threads executes processPendingDerefs(). Framework has added a patch to detect this kind of symptom.

Code research:

[IPCThreadState.cpp]
void IPCThreadState::joinThreadPool(bool isMain)

do {
/// [framework] begin, mark_chen, 2014/07/11, detect invalid transaction
mCanTransact = false;
processPendingDerefs();              ////// unexpected BC_XXXX command is issued from here
mCanTransact = true;
/// [framework] end, mark_chen, 2014/07/11

// now get the next command to be processed, waiting if necessary
result = getAndExecuteCommand();

if (result < NO_ERROR && result != TIMED_OUT && result != -ECONNREFUSED && result != -EBADF) {
ALOGE("getAndExecuteCommand(fd=%d) returned unexpected error %d, aborting",
mProcess->mDriverFD, result);
abort();
}

// Let this thread exit the thread pool if it is no longer
// needed and it is not the main process thread.
if(result == TIMED_OUT && !isMain) {
break;
}
} while (result != -ECONNREFUSED && result != -EBADF);
...

status_t IPCThreadState::getAndExecuteCommand()
{
status_t result;
int32_t cmd;

result = talkWithDriver();
if (result >= NO_ERROR) {
size_t IN = mIn.dataAvail();
if (IN < sizeof(int32_t)) return result;
cmd = mIn.readInt32();
IF_LOG_COMMANDS() {
alog << "Processing top-level Command: "
<< getReturnString(cmd) << endl;
}

result = executeCommand(cmd);

status_t IPCThreadState::executeCommand(int32_t cmd)
{
BBinder* obj;
RefBase::weakref_type* refs;
status_t result = NO_ERROR;

switch (cmd) {
…
default:
/// [framework] begin, mark_chen, Legacy, add log for debug
//printf("*** BAD COMMAND %d received from Binder driver\n", cmd);
ALOGE("*** BAD COMMAND %d received from Binder driver\n", cmd);
/// [framework] end, mark_chen, Legacy
result = UNKNOWN_ERROR;
break;
}

Framework Debug Log patch:

[IPCThreadState.cpp]
void IPCThreadState::joinThreadPool(bool isMain)
...
do {
/// [framework] begin, mark_chen, 2014/07/11, detect invalid transaction
mCanTransact = false;
processPendingDerefs();
mCanTransact = true;
/// [framework] end, mark_chen, 2014/07/11

status_t IPCThreadState::transact(int32_t handle,
uint32_t code, const Parcel& data,
Parcel* reply, uint32_t flags)
{
/// [framework] begin, mark_chen, 2014/07/11, detect invalid transaction
if (!mCanTransact) {
#if (HTC_SECURITY_DEBUG_FLAG == 1)
TextOutput::Bundle _b(alog);
alog << "Invalid BC_TRANSACTION " << (void*)pthread_self() << " / hand "
<< handle << " / code " << TypeCode(code) << ": "
<< indent << data << dedent << endl;
#endif
}