Merge "libbinder: attachObject* APIs [[nodiscard]]"

4 files changed, 25 insertions, 20 deletions

diff --git a/libs/binder/ServiceManagerHost.cpp b/libs/binder/ServiceManagerHost.cpp
index 07f5778deb..1c2f9b4314 100644
--- a/libs/binder/ServiceManagerHost.cpp
+++ b/libs/binder/ServiceManagerHost.cpp
@@ -167,9 +167,12 @@ sp<IBinder> getDeviceService(std::vector<std::string>&& serviceDispatcherArgs) {
         ALOGE("RpcSession::getRootObject returns nullptr");
         return nullptr;
     }
-    binder->attachObject(kDeviceServiceExtraId,
-                         static_cast<void*>(new CommandResult(std::move(*result))), nullptr,
-                         &cleanupCommandResult);
+
+    LOG_ALWAYS_FATAL_IF(
+            nullptr !=
+            binder->attachObject(kDeviceServiceExtraId,
+                                 static_cast<void*>(new CommandResult(std::move(*result))), nullptr,
+                                 &cleanupCommandResult));
     return binder;
 }
 
diff --git a/libs/binder/include/binder/IBinder.h b/libs/binder/include/binder/IBinder.h
index c484d8347c..43fc5ff1a9 100644
--- a/libs/binder/include/binder/IBinder.h
+++ b/libs/binder/include/binder/IBinder.h
@@ -259,21 +259,20 @@ public:
      * but calls from different threads are allowed to be interleaved.
      *
      * This returns the object which is already attached. If this returns a
-     * non-null value, it means that attachObject failed. TODO(b/192023359):
-     * remove logs and add [[nodiscard]]
+     * non-null value, it means that attachObject failed (a given objectID can
+     * only be used once).
      */
-    virtual void* attachObject(const void* objectID, void* object, void* cleanupCookie,
-                               object_cleanup_func func) = 0;
+    [[nodiscard]] virtual void* attachObject(const void* objectID, void* object,
+                                             void* cleanupCookie, object_cleanup_func func) = 0;
     /**
      * Returns object attached with attachObject.
      */
-    virtual void*           findObject(const void* objectID) const = 0;
+    [[nodiscard]] virtual void* findObject(const void* objectID) const = 0;
     /**
      * Returns object attached with attachObject, and detaches it. This does not
-     * delete the object. This is equivalent to using attachObject to attach a null
-     * object.
+     * delete the object.
      */
-    virtual void* detachObject(const void* objectID) = 0;
+    [[nodiscard]] virtual void* detachObject(const void* objectID) = 0;
 
     /**
      * Use the lock that this binder contains internally. For instance, this can
diff --git a/libs/binder/ndk/ibinder.cpp b/libs/binder/ndk/ibinder.cpp
index 266ef379bc..b89714ad58 100644
--- a/libs/binder/ndk/ibinder.cpp
+++ b/libs/binder/ndk/ibinder.cpp
@@ -47,7 +47,8 @@ static void* kValue = static_cast<void*>(new bool{true});
 void clean(const void* /*id*/, void* /*obj*/, void* /*cookie*/){/* do nothing */};
 
 static void attach(const sp<IBinder>& binder) {
-    binder->attachObject(kId, kValue, nullptr /*cookie*/, clean);
+    // can only attach once
+    CHECK_EQ(nullptr, binder->attachObject(kId, kValue, nullptr /*cookie*/, clean));
 }
 static bool has(const sp<IBinder>& binder) {
     return binder != nullptr && binder->findObject(kId) == kValue;
diff --git a/libs/binder/tests/unit_fuzzers/IBinderFuzzFunctions.h b/libs/binder/tests/unit_fuzzers/IBinderFuzzFunctions.h
index 626b7585f7..4a0aebac43 100644
--- a/libs/binder/tests/unit_fuzzers/IBinderFuzzFunctions.h
+++ b/libs/binder/tests/unit_fuzzers/IBinderFuzzFunctions.h
@@ -62,20 +62,22 @@ static const std::vector<std::function<void(FuzzedDataProvider*, IBinder*)>> gIB
              object = fdp->ConsumeIntegral<uint32_t>();
              cleanup_cookie = fdp->ConsumeIntegral<uint32_t>();
              IBinder::object_cleanup_func func = IBinder::object_cleanup_func();
-             ibinder->attachObject(fdp->ConsumeBool() ? reinterpret_cast<void*>(&objectID)
-                                                      : nullptr,
-                                   fdp->ConsumeBool() ? reinterpret_cast<void*>(&object) : nullptr,
-                                   fdp->ConsumeBool() ? reinterpret_cast<void*>(&cleanup_cookie)
-                                                      : nullptr,
-                                   func);
+             (void)ibinder->attachObject(fdp->ConsumeBool() ? reinterpret_cast<void*>(&objectID)
+                                                            : nullptr,
+                                         fdp->ConsumeBool() ? reinterpret_cast<void*>(&object)
+                                                            : nullptr,
+                                         fdp->ConsumeBool()
+                                                 ? reinterpret_cast<void*>(&cleanup_cookie)
+                                                 : nullptr,
+                                         func);
          },
          [](FuzzedDataProvider* fdp, IBinder* ibinder) -> void {
              uint32_t id = fdp->ConsumeIntegral<uint32_t>();
-             ibinder->findObject(reinterpret_cast<void*>(&id));
+             (void)ibinder->findObject(reinterpret_cast<void*>(&id));
          },
          [](FuzzedDataProvider* fdp, IBinder* ibinder) -> void {
              uint32_t id = fdp->ConsumeIntegral<uint32_t>();
-             ibinder->detachObject(reinterpret_cast<void*>(&id));
+             (void)ibinder->detachObject(reinterpret_cast<void*>(&id));
          },
          [](FuzzedDataProvider* fdp, IBinder* ibinder) -> void {
              uint32_t code = fdp->ConsumeIntegral<uint32_t>();