libbinder_ndk: fix failure when dump/shell are unset

People directly using libbinder_ndk functions who didn't create a debug
dump function function would fail to initialize that pointer, and
potentially crash. Those who didn't create a shell function were
guaranteed to crash. This wasn't noticed because the C++ wrappers which
are the recommended way to use libbinder_ndk always set these functions.

Bug: 161812320
Test: unit tests

Merged-In: I1f6909531bc640097f3f48c4a558fd03f2fa62cb
Change-Id: I1f6909531bc640097f3f48c4a558fd03f2fa62cb

5 files changed, 40 insertions, 7 deletions

diff --git a/libs/binder/ndk/ibinder.cpp b/libs/binder/ndk/ibinder.cpp
index 649faa1c76..919150d740 100644
--- a/libs/binder/ndk/ibinder.cpp
+++ b/libs/binder/ndk/ibinder.cpp
@@ -161,7 +161,7 @@ status_t ABBinder::onTransact(transaction_code_t code, const Parcel& data, Parce
 
         binder_status_t status = getClass()->onTransact(this, code, &in, &out);
         return PruneStatusT(status);
-    } else if (code == SHELL_COMMAND_TRANSACTION) {
+    } else if (code == SHELL_COMMAND_TRANSACTION && getClass()->handleShellCommand != nullptr) {
         int in = data.readFileDescriptor();
         int out = data.readFileDescriptor();
         int err = data.readFileDescriptor();
diff --git a/libs/binder/ndk/ibinder_internal.h b/libs/binder/ndk/ibinder_internal.h
index 57794279f2..902fe7934d 100644
--- a/libs/binder/ndk/ibinder_internal.h
+++ b/libs/binder/ndk/ibinder_internal.h
@@ -110,13 +110,13 @@ struct AIBinder_Class {
     const ::android::String16& getInterfaceDescriptor() const { return mInterfaceDescriptor; }
 
     // required to be non-null, implemented for every class
-    const AIBinder_Class_onCreate onCreate;
-    const AIBinder_Class_onDestroy onDestroy;
-    const AIBinder_Class_onTransact onTransact;
+    const AIBinder_Class_onCreate onCreate = nullptr;
+    const AIBinder_Class_onDestroy onDestroy = nullptr;
+    const AIBinder_Class_onTransact onTransact = nullptr;
 
     // optional methods for a class
-    AIBinder_onDump onDump;
-    AIBinder_handleShellCommand handleShellCommand;
+    AIBinder_onDump onDump = nullptr;
+    AIBinder_handleShellCommand handleShellCommand = nullptr;
 
    private:
     // This must be a String16 since BBinder virtual getInterfaceDescriptor returns a reference to
diff --git a/libs/binder/ndk/test/iface.cpp b/libs/binder/ndk/test/iface.cpp
index 64832f3081..a5889856fc 100644
--- a/libs/binder/ndk/test/iface.cpp
+++ b/libs/binder/ndk/test/iface.cpp
@@ -118,7 +118,7 @@ IFoo::~IFoo() {
     AIBinder_Weak_delete(mWeakBinder);
 }
 
-binder_status_t IFoo::addService(const char* instance) {
+AIBinder* IFoo::getBinder() {
     AIBinder* binder = nullptr;
 
     if (mWeakBinder != nullptr) {
@@ -132,8 +132,18 @@ binder_status_t IFoo::addService(const char* instance) {
             AIBinder_Weak_delete(mWeakBinder);
         }
         mWeakBinder = AIBinder_Weak_new(binder);
+
+        // WARNING: it is important that this class does not implement debug or
+        // shell functions because it does not use special C++ wrapper
+        // functions, and so this is how we test those functions.
     }
 
+    return binder;
+}
+
+binder_status_t IFoo::addService(const char* instance) {
+    AIBinder* binder = getBinder();
+
     binder_status_t status = AServiceManager_addService(binder, instance);
     // Strong references we care about kept by remote process
     AIBinder_decStrong(binder);
diff --git a/libs/binder/ndk/test/include/iface/iface.h b/libs/binder/ndk/test/include/iface/iface.h
index cdf5493216..d9dd64b8a6 100644
--- a/libs/binder/ndk/test/include/iface/iface.h
+++ b/libs/binder/ndk/test/include/iface/iface.h
@@ -30,6 +30,9 @@ class IFoo : public virtual ::android::RefBase {
 
     static AIBinder_Class* kClass;
 
+    // binder representing this interface with one reference count
+    AIBinder* getBinder();
+
     // Takes ownership of IFoo
     binder_status_t addService(const char* instance);
     static ::android::sp<IFoo> getService(const char* instance, AIBinder** outBinder = nullptr);
diff --git a/libs/binder/ndk/test/libbinder_ndk_unit_test.cpp b/libs/binder/ndk/test/libbinder_ndk_unit_test.cpp
index fd30d87c76..aaf36b97a4 100644
--- a/libs/binder/ndk/test/libbinder_ndk_unit_test.cpp
+++ b/libs/binder/ndk/test/libbinder_ndk_unit_test.cpp
@@ -126,6 +126,26 @@ TEST(NdkBinder, CheckServiceThatDoesExist) {
     AIBinder_decStrong(binder);
 }
 
+TEST(NdkBinder, UnimplementedDump) {
+    sp<IFoo> foo = IFoo::getService(IFoo::kSomeInstanceName);
+    ASSERT_NE(foo, nullptr);
+    AIBinder* binder = foo->getBinder();
+    EXPECT_EQ(OK, AIBinder_dump(binder, STDOUT_FILENO, nullptr, 0));
+    AIBinder_decStrong(binder);
+}
+
+TEST(NdkBinder, UnimplementedShell) {
+    // libbinder_ndk doesn't support calling shell, so we are calling from the
+    // libbinder across processes to the NDK service which doesn't implement
+    // shell
+    static const sp<android::IServiceManager> sm(android::defaultServiceManager());
+    sp<IBinder> testService = sm->getService(String16(IFoo::kSomeInstanceName));
+
+    Vector<String16> argsVec;
+    EXPECT_EQ(OK, IBinder::shellCommand(testService, STDIN_FILENO, STDOUT_FILENO, STDERR_FILENO,
+                                        argsVec, nullptr, nullptr));
+}
+
 TEST(NdkBinder, DoubleNumber) {
     sp<IFoo> foo = IFoo::getService(IFoo::kSomeInstanceName);
     ASSERT_NE(foo, nullptr);