libbinder: RPC disallow nested oneway transactions
Previously, nested transactions were accidentally allowed while
processing oneway transactions. This changes things so that nested
transactions are only explicitly allowed when a synchronous transaction
is being processed (like how kernel binder is).
Future considerations: this CL makes it more explicit that we allow
refcount transactions as part of nested transactions. This is okay
because 'drainCommands' will process these, but there might be some
delay. We could make refcount behavior nicer if we always preferred
using an active threadpool (if one is available) to process them.
Bug: 167966510
Test: binderRpcTest
Change-Id: Iaeb472896654ff4bcd75b20394f8f3230febaabf
diff --git a/libs/binder/tests/binderRpcTest.cpp b/libs/binder/tests/binderRpcTest.cpp
index a79295a..69682b0 100644
--- a/libs/binder/tests/binderRpcTest.cpp
+++ b/libs/binder/tests/binderRpcTest.cpp
@@ -214,7 +214,8 @@
if (delayed) {
std::thread([=]() {
ALOGE("Executing delayed callback: '%s'", value.c_str());
- (void)doCallback(callback, oneway, false, value);
+ Status status = doCallback(callback, oneway, false, value);
+ ALOGE("Delayed callback status: '%s'", status.toString8().c_str());
}).detach();
return Status::ok();
}
@@ -226,6 +227,11 @@
return callback->sendCallback(value);
}
+ Status doCallbackAsync(const sp<IBinderRpcCallback>& callback, bool oneway, bool delayed,
+ const std::string& value) override {
+ return doCallback(callback, oneway, delayed, value);
+ }
+
Status die(bool cleanup) override {
if (cleanup) {
exit(1);
@@ -978,31 +984,42 @@
TEST_P(BinderRpc, Callbacks) {
const static std::string kTestString = "good afternoon!";
- for (bool oneway : {true, false}) {
- for (bool delayed : {true, false}) {
- auto proc = createRpcTestSocketServerProcess(1, 1, 1);
- auto cb = sp<MyBinderRpcCallback>::make();
+ for (bool callIsOneway : {true, false}) {
+ for (bool callbackIsOneway : {true, false}) {
+ for (bool delayed : {true, false}) {
+ auto proc = createRpcTestSocketServerProcess(1, 1, 1);
+ auto cb = sp<MyBinderRpcCallback>::make();
- EXPECT_OK(proc.rootIface->doCallback(cb, oneway, delayed, kTestString));
+ if (callIsOneway) {
+ EXPECT_OK(proc.rootIface->doCallbackAsync(cb, callbackIsOneway, delayed,
+ kTestString));
+ } else {
+ EXPECT_OK(
+ proc.rootIface->doCallback(cb, callbackIsOneway, delayed, kTestString));
+ }
- using std::literals::chrono_literals::operator""s;
- std::unique_lock<std::mutex> _l(cb->mMutex);
- cb->mCv.wait_for(_l, 1s, [&] { return !cb->mValues.empty(); });
+ using std::literals::chrono_literals::operator""s;
+ std::unique_lock<std::mutex> _l(cb->mMutex);
+ cb->mCv.wait_for(_l, 1s, [&] { return !cb->mValues.empty(); });
- EXPECT_EQ(cb->mValues.size(), 1) << "oneway: " << oneway << "delayed: " << delayed;
- if (cb->mValues.empty()) continue;
- EXPECT_EQ(cb->mValues.at(0), kTestString)
- << "oneway: " << oneway << "delayed: " << delayed;
+ EXPECT_EQ(cb->mValues.size(), 1)
+ << "callIsOneway: " << callIsOneway
+ << " callbackIsOneway: " << callbackIsOneway << " delayed: " << delayed;
+ if (cb->mValues.empty()) continue;
+ EXPECT_EQ(cb->mValues.at(0), kTestString)
+ << "callIsOneway: " << callIsOneway
+ << " callbackIsOneway: " << callbackIsOneway << " delayed: " << delayed;
- // since we are severing the connection, we need to go ahead and
- // tell the server to shutdown and exit so that waitpid won't hang
- EXPECT_OK(proc.rootIface->scheduleShutdown());
+ // since we are severing the connection, we need to go ahead and
+ // tell the server to shutdown and exit so that waitpid won't hang
+ EXPECT_OK(proc.rootIface->scheduleShutdown());
- // since this session has a reverse connection w/ a threadpool, we
- // need to manually shut it down
- EXPECT_TRUE(proc.proc.sessions.at(0).session->shutdownAndWait(true));
+ // since this session has a reverse connection w/ a threadpool, we
+ // need to manually shut it down
+ EXPECT_TRUE(proc.proc.sessions.at(0).session->shutdownAndWait(true));
- proc.expectAlreadyShutdown = true;
+ proc.expectAlreadyShutdown = true;
+ }
}
}
}