Merge "libbinder_rs: Treat previously associated Binder as remote"

9 files changed, 358 insertions, 18 deletions

diff --git a/libs/binder/TEST_MAPPING b/libs/binder/TEST_MAPPING
index 1cfb560f82..437581884f 100644
--- a/libs/binder/TEST_MAPPING
+++ b/libs/binder/TEST_MAPPING
@@ -56,6 +56,9 @@
     },
     {
       "name": "rustBinderTest"
+    },
+    {
+      "name": "binderRustNdkInteropTest"
     }
   ]
 }
diff --git a/libs/binder/rust/src/binder.rs b/libs/binder/rust/src/binder.rs
index 037ee95683..b558f27e06 100644
--- a/libs/binder/rust/src/binder.rs
+++ b/libs/binder/rust/src/binder.rs
@@ -21,7 +21,8 @@ use crate::parcel::Parcel;
 use crate::proxy::{DeathRecipient, SpIBinder};
 use crate::sys;
 
-use std::ffi::{c_void, CString};
+use std::ffi::{c_void, CStr, CString};
+use std::os::raw::c_char;
 use std::os::unix::io::AsRawFd;
 use std::ptr;
 
@@ -205,6 +206,22 @@ impl InterfaceClass {
     pub(crate) unsafe fn from_ptr(ptr: *const sys::AIBinder_Class) -> InterfaceClass {
         InterfaceClass(ptr)
     }
+
+    /// Get the interface descriptor string of this class.
+    pub fn get_descriptor(&self) -> String {
+        unsafe {
+            // SAFETY: The descriptor returned by AIBinder_Class_getDescriptor
+            // is always a two-byte null terminated sequence of u16s. Thus, we
+            // can continue reading from the pointer until we hit a null value,
+            // and this pointer can be a valid slice if the slice length is <=
+            // the number of u16 elements before the null terminator.
+
+            let raw_descriptor: *const c_char = sys::AIBinder_Class_getDescriptor(self.0);
+            CStr::from_ptr(raw_descriptor).to_str()
+                .expect("Expected valid UTF-8 string from AIBinder_Class_getDescriptor")
+                .into()
+        }
+    }
 }
 
 impl From<InterfaceClass> for *const sys::AIBinder_Class {
@@ -507,12 +524,7 @@ macro_rules! declare_binder_interface {
             }
 
             fn from_binder(mut binder: $crate::SpIBinder) -> $crate::Result<Self> {
-                use $crate::AssociateClass;
-                if binder.associate_class(<$native as $crate::Remotable>::get_class()) {
-                    Ok(Self { binder, $($fname: $finit),* })
-                } else {
-                    Err($crate::StatusCode::BAD_TYPE)
-                }
+                Ok(Self { binder, $($fname: $finit),* })
             }
         }
 
@@ -567,16 +579,33 @@ macro_rules! declare_binder_interface {
         impl $crate::FromIBinder for dyn $interface {
             fn try_from(mut ibinder: $crate::SpIBinder) -> $crate::Result<Box<dyn $interface>> {
                 use $crate::AssociateClass;
-                if !ibinder.associate_class(<$native as $crate::Remotable>::get_class()) {
-                    return Err($crate::StatusCode::BAD_TYPE.into());
-                }
 
-                let service: $crate::Result<$crate::Binder<$native>> = std::convert::TryFrom::try_from(ibinder.clone());
-                if let Ok(service) = service {
-                    Ok(Box::new(service))
-                } else {
-                    Ok(Box::new(<$proxy as $crate::Proxy>::from_binder(ibinder)?))
+                let existing_class = ibinder.get_class();
+                if let Some(class) = existing_class {
+                    if class != <$native as $crate::Remotable>::get_class() &&
+                        class.get_descriptor() == <$native as $crate::Remotable>::get_descriptor()
+                    {
+                        // The binder object's descriptor string matches what we
+                        // expect. We still need to treat this local or already
+                        // associated object as remote, because we can't cast it
+                        // into a Rust service object without a matching class
+                        // pointer.
+                        return Ok(Box::new(<$proxy as $crate::Proxy>::from_binder(ibinder)?));
+                    }
+                } else if ibinder.associate_class(<$native as $crate::Remotable>::get_class()) {
+                    let service: $crate::Result<$crate::Binder<$native>> =
+                        std::convert::TryFrom::try_from(ibinder.clone());
+                    if let Ok(service) = service {
+                        // We were able to associate with our expected class and
+                        // the service is local.
+                        return Ok(Box::new(service));
+                    } else {
+                        // Service is remote
+                        return Ok(Box::new(<$proxy as $crate::Proxy>::from_binder(ibinder)?));
+                    }
                 }
+
+                Err($crate::StatusCode::BAD_TYPE.into())
             }
         }
 
diff --git a/libs/binder/rust/src/proxy.rs b/libs/binder/rust/src/proxy.rs
index 5002fc6d83..485bb422fa 100644
--- a/libs/binder/rust/src/proxy.rs
+++ b/libs/binder/rust/src/proxy.rs
@@ -91,7 +91,7 @@ impl SpIBinder {
 
     /// Return the interface class of this binder object, if associated with
     /// one.
-    pub(crate) fn get_class(&mut self) -> Option<InterfaceClass> {
+    pub fn get_class(&mut self) -> Option<InterfaceClass> {
         unsafe {
             // Safety: `SpIBinder` guarantees that it always contains a valid
             // `AIBinder` pointer. `AIBinder_getClass` returns either a null
diff --git a/libs/binder/rust/tests/Android.bp b/libs/binder/rust/tests/Android.bp
index 3db40ba92d..5ae9c53077 100644
--- a/libs/binder/rust/tests/Android.bp
+++ b/libs/binder/rust/tests/Android.bp
@@ -30,3 +30,52 @@ rust_test {
     auto_gen_config: false,
     test_suites: ["general-tests"],
 }
+
+cc_test {
+    name: "binderRustNdkInteropTest",
+    srcs: [
+        "binderRustNdkInteropTest.cpp",
+    ],
+    shared_libs: [
+        "libbinder",
+        "libbinder_ndk",
+    ],
+    static_libs: [
+        "IBinderRustNdkInteropTest-ndk_platform",
+        "libbinder_ndk_rust_interop",
+    ],
+    test_suites: ["general-tests"],
+    require_root: true,
+
+    // rustBinderTestService uses a custom config
+    auto_gen_config: true,
+}
+
+aidl_interface {
+    name: "IBinderRustNdkInteropTest",
+    unstable: true,
+    srcs: [
+        "IBinderRustNdkInteropTest.aidl",
+        "IBinderRustNdkInteropTestOther.aidl",
+    ],
+    backend: {
+        ndk: {
+            enabled: true,
+        },
+        rust: {
+            enabled: true,
+        },
+    },
+}
+
+rust_ffi_static {
+    name: "libbinder_ndk_rust_interop",
+    crate_name: "binder_ndk_rust_interop",
+    srcs: [
+        "ndk_rust_interop.rs",
+    ],
+    rustlibs: [
+        "libbinder_rs",
+        "IBinderRustNdkInteropTest-rust",
+    ],
+}
diff --git a/libs/binder/rust/tests/IBinderRustNdkInteropTest.aidl b/libs/binder/rust/tests/IBinderRustNdkInteropTest.aidl
new file mode 100644
index 0000000000..7f5e837d28
--- /dev/null
+++ b/libs/binder/rust/tests/IBinderRustNdkInteropTest.aidl
@@ -0,0 +1,19 @@
+/*
+ * Copyright (C) 2020 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+interface IBinderRustNdkInteropTest {
+    @utf8InCpp String echo(@utf8InCpp String str);
+}
diff --git a/libs/binder/rust/tests/IBinderRustNdkInteropTestOther.aidl b/libs/binder/rust/tests/IBinderRustNdkInteropTestOther.aidl
new file mode 100644
index 0000000000..82a03237a4
--- /dev/null
+++ b/libs/binder/rust/tests/IBinderRustNdkInteropTestOther.aidl
@@ -0,0 +1,19 @@
+/*
+ * Copyright (C) 2020 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+interface IBinderRustNdkInteropTestOther {
+    @utf8InCpp String echo(@utf8InCpp String str);
+}
diff --git a/libs/binder/rust/tests/binderRustNdkInteropTest.cpp b/libs/binder/rust/tests/binderRustNdkInteropTest.cpp
new file mode 100644
index 0000000000..59ca6edf12
--- /dev/null
+++ b/libs/binder/rust/tests/binderRustNdkInteropTest.cpp
@@ -0,0 +1,78 @@
+/*
+ * Copyright (C) 2020 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <aidl/BnBinderRustNdkInteropTest.h>
+#include <aidl/IBinderRustNdkInteropTest.h>
+#include <android/binder_ibinder.h>
+#include <android/binder_manager.h>
+#include <android/binder_process.h>
+#include <binder/Status.h>
+#include <gtest/gtest.h>
+
+using namespace android;
+using ::ndk::ScopedAStatus;
+using ::ndk::SharedRefBase;
+using ::ndk::SpAIBinder;
+
+static const char* kNdkServerName = "NdkServer-BinderRustNdkInteropTest";
+static const char* kRustServerName = "RustServer-BinderRustNdkInteropTest";
+
+extern "C" {
+int rust_call_ndk(const char* service_name);
+int rust_start_service(const char* service_name);
+}
+
+class NdkServer : public aidl::BnBinderRustNdkInteropTest {
+    ScopedAStatus echo(const std::string& in, std::string* out) override {
+        *out = in;
+        return ScopedAStatus::ok();
+    }
+};
+
+TEST(RustNdkInterop, RustCanCallNdk) {
+    ASSERT_EQ(STATUS_OK, rust_call_ndk(kNdkServerName));
+}
+
+TEST(RustNdkInterop, NdkCanCallRust) {
+    ASSERT_EQ(STATUS_OK, rust_start_service(kRustServerName));
+
+    SpAIBinder binder = SpAIBinder(AServiceManager_checkService(kRustServerName));
+    ASSERT_NE(nullptr, binder.get());
+    EXPECT_EQ(STATUS_OK, AIBinder_ping(binder.get()));
+
+    auto interface = aidl::IBinderRustNdkInteropTest::fromBinder(binder);
+    // TODO(b/167723746): this test requires that fromBinder allow association
+    // with an already associated local binder by treating it as remote.
+    EXPECT_EQ(interface, nullptr);
+
+    // std::string in("testing");
+    // std::string out;
+    // EXPECT_TRUE(interface->echo(in, &out).isOk());
+    // EXPECT_EQ(in, out);
+}
+
+int main(int argc, char** argv) {
+    ::testing::InitGoogleTest(&argc, argv);
+
+    // so we can host a client and service concurrently
+    ABinderProcess_setThreadPoolMaxThreadCount(1);
+    ABinderProcess_startThreadPool();
+
+    std::shared_ptr<NdkServer> ndkServer = SharedRefBase::make<NdkServer>();
+    EXPECT_EQ(STATUS_OK, AServiceManager_addService(ndkServer->asBinder().get(), kNdkServerName));
+
+    return RUN_ALL_TESTS();
+}
diff --git a/libs/binder/rust/tests/integration.rs b/libs/binder/rust/tests/integration.rs
index 953d328d23..497361a0ed 100644
--- a/libs/binder/rust/tests/integration.rs
+++ b/libs/binder/rust/tests/integration.rs
@@ -173,6 +173,30 @@ impl ITest for Binder<BnTest> {
     }
 }
 
+/// Trivial testing binder interface
+pub trait ITestSameDescriptor: Interface {}
+
+declare_binder_interface! {
+    ITestSameDescriptor["android.os.ITest"] {
+        native: BnTestSameDescriptor(on_transact_same_descriptor),
+        proxy: BpTestSameDescriptor,
+    }
+}
+
+fn on_transact_same_descriptor(
+    _service: &dyn ITestSameDescriptor,
+    _code: TransactionCode,
+    _data: &Parcel,
+    _reply: &mut Parcel,
+) -> binder::Result<()> {
+    Ok(())
+}
+
+impl ITestSameDescriptor for BpTestSameDescriptor {}
+
+impl ITestSameDescriptor for Binder<BnTestSameDescriptor> {}
+
+
 #[cfg(test)]
 mod tests {
     use selinux_bindgen as selinux_sys;
@@ -185,9 +209,9 @@ mod tests {
     use std::thread;
     use std::time::Duration;
 
-    use binder::{DeathRecipient, FromIBinder, IBinder, SpIBinder, StatusCode};
+    use binder::{Binder, DeathRecipient, FromIBinder, IBinder, Interface, SpIBinder, StatusCode};
 
-    use super::{ITest, RUST_SERVICE_BINARY};
+    use super::{BnTest, ITest, ITestSameDescriptor, RUST_SERVICE_BINARY, TestService};
 
     pub struct ScopedServiceProcess(Child);
 
@@ -435,4 +459,27 @@ mod tests {
             assert_eq!(extension.test().unwrap(), extension_name);
         }
     }
+
+    /// Test re-associating a local binder object with a different class.
+    ///
+    /// This is needed because different binder service (e.g. NDK vs Rust)
+    /// implementations are incompatible and must not be interchanged. A local
+    /// service with the same descriptor string but a different class pointer
+    /// may have been created by an NDK service and is therefore incompatible
+    /// with the Rust service implementation. It must be treated as remote and
+    /// all API calls parceled and sent through transactions.
+    ///
+    /// Further tests of this behavior with the C NDK and Rust API are in
+    /// rust_ndk_interop.rs
+    #[test]
+    fn associate_existing_class() {
+        let service = Binder::new(BnTest(Box::new(TestService {
+            s: "testing_service".to_string(),
+        })));
+
+        // This should succeed although we will have to treat the service as
+        // remote.
+        let _interface: Box<dyn ITestSameDescriptor> = FromIBinder::try_from(service.as_binder())
+            .expect("Could not re-interpret service as the ITestSameDescriptor interface");
+    }
 }
diff --git a/libs/binder/rust/tests/ndk_rust_interop.rs b/libs/binder/rust/tests/ndk_rust_interop.rs
new file mode 100644
index 0000000000..70a6dc0788
--- /dev/null
+++ b/libs/binder/rust/tests/ndk_rust_interop.rs
@@ -0,0 +1,96 @@
+/*
+ * Copyright (C) 2020 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+//! Rust Binder NDK interop tests
+
+use std::ffi::CStr;
+use std::os::raw::{c_char, c_int};
+use ::IBinderRustNdkInteropTest::binder::{self, Interface, StatusCode};
+use ::IBinderRustNdkInteropTest::aidl::IBinderRustNdkInteropTest::{
+    BnBinderRustNdkInteropTest, IBinderRustNdkInteropTest,
+};
+use ::IBinderRustNdkInteropTest::aidl::IBinderRustNdkInteropTestOther::{
+    IBinderRustNdkInteropTestOther,
+};
+
+/// Look up the provided AIDL service and call its echo method.
+///
+/// # Safety
+///
+/// service_name must be a valid, non-null C-style string (null-terminated).
+#[no_mangle]
+pub unsafe extern "C" fn rust_call_ndk(service_name: *const c_char) -> c_int {
+    let service_name = CStr::from_ptr(service_name).to_str().unwrap();
+
+    // The Rust class descriptor pointer will not match the NDK one, but the
+    // descriptor strings match so this needs to still associate.
+    let service: Box<dyn IBinderRustNdkInteropTest> = match binder::get_interface(service_name) {
+        Err(e) => {
+            eprintln!("Could not find Ndk service {}: {:?}", service_name, e);
+            return StatusCode::NAME_NOT_FOUND as c_int;
+        }
+        Ok(service) => service,
+    };
+
+    match service.echo("testing") {
+        Ok(s) => if s != "testing" {
+            return StatusCode::BAD_VALUE as c_int;
+        },
+        Err(e) => return e.into(),
+    }
+
+    // Try using the binder service through the wrong interface type
+    let wrong_service: Result<Box<dyn IBinderRustNdkInteropTestOther>, StatusCode> =
+        binder::get_interface(service_name);
+    match wrong_service {
+        Err(e) if e == StatusCode::BAD_TYPE => {}
+        Err(e) => {
+            eprintln!("Trying to use a service via the wrong interface errored with unexpected error {:?}", e);
+            return e as c_int;
+        }
+        Ok(_) => {
+            eprintln!("We should not be allowed to use a service via the wrong interface");
+            return StatusCode::BAD_TYPE as c_int;
+        }
+    }
+
+    StatusCode::OK as c_int
+}
+
+struct Service;
+
+impl Interface for Service {}
+
+impl IBinderRustNdkInteropTest for Service {
+    fn echo(&self, s: &str) -> binder::Result<String> {
+        Ok(s.to_string())
+    }
+}
+
+/// Start the interop Echo test service with the given service name.
+///
+/// # Safety
+///
+/// service_name must be a valid, non-null C-style string (null-terminated).
+#[no_mangle]
+pub unsafe extern "C" fn rust_start_service(service_name: *const c_char) -> c_int {
+    let service_name = CStr::from_ptr(service_name).to_str().unwrap();
+    let service = BnBinderRustNdkInteropTest::new_binder(Service);
+    match binder::add_service(&service_name, service.as_binder()) {
+        Ok(_) => StatusCode::OK as c_int,
+        Err(e) => e as c_int,
+    }
+}