Fix or suppress tidy warnings-as-errors.
Use std::map instead of KeyedVector (deprecated) in order to avoid
unnecessary (and implicit) initialization of the value type. KeyedVector
does it even when only the key is neeed (e.g. indexOfKey). std::map
doesn't have such a problem.
Bug: 222775179
Test: unset WITH_TIDY; CLANG_ANALYZER_CHECKS=1 make -k
tidy-frameworks-native-libs-binder
Change-Id: I548fc96a34bac9c7135e206983150948dbca57d4
diff --git a/libs/binder/Binder.cpp b/libs/binder/Binder.cpp
index 0970ca5..01b25d3 100644
--- a/libs/binder/Binder.cpp
+++ b/libs/binder/Binder.cpp
@@ -19,6 +19,7 @@
#include <atomic>
#include <set>
+#include <android-base/logging.h>
#include <android-base/unique_fd.h>
#include <binder/BpBinder.h>
#include <binder/IInterface.h>
@@ -281,9 +282,11 @@
err = pingBinder();
break;
case EXTENSION_TRANSACTION:
+ CHECK(reply != nullptr);
err = reply->writeStrongBinder(getExtension());
break;
case DEBUG_PID_TRANSACTION:
+ CHECK(reply != nullptr);
err = reply->writeInt32(getDebugPid());
break;
case SET_RPC_CLIENT_TRANSACTION: {
@@ -590,6 +593,7 @@
{
switch (code) {
case INTERFACE_TRANSACTION:
+ CHECK(reply != nullptr);
reply->writeString16(getInterfaceDescriptor());
return NO_ERROR;
diff --git a/libs/binder/BpBinder.cpp b/libs/binder/BpBinder.cpp
index 056ef0a..921e57c 100644
--- a/libs/binder/BpBinder.cpp
+++ b/libs/binder/BpBinder.cpp
@@ -72,29 +72,29 @@
e.cleanupCookie = cleanupCookie;
e.func = func;
- if (ssize_t idx = mObjects.indexOfKey(objectID); idx >= 0) {
+ if (mObjects.find(objectID) != mObjects.end()) {
ALOGI("Trying to attach object ID %p to binder ObjectManager %p with object %p, but object "
"ID already in use",
objectID, this, object);
- return mObjects[idx].object;
+ return mObjects[objectID].object;
}
- mObjects.add(objectID, e);
+ mObjects.insert({objectID, e});
return nullptr;
}
void* BpBinder::ObjectManager::find(const void* objectID) const
{
- const ssize_t i = mObjects.indexOfKey(objectID);
- if (i < 0) return nullptr;
- return mObjects.valueAt(i).object;
+ auto i = mObjects.find(objectID);
+ if (i == mObjects.end()) return nullptr;
+ return i->second.object;
}
void* BpBinder::ObjectManager::detach(const void* objectID) {
- ssize_t idx = mObjects.indexOfKey(objectID);
- if (idx < 0) return nullptr;
- void* value = mObjects[idx].object;
- mObjects.removeItemsAt(idx, 1);
+ auto i = mObjects.find(objectID);
+ if (i == mObjects.end()) return nullptr;
+ void* value = i->second.object;
+ mObjects.erase(i);
return value;
}
@@ -102,10 +102,10 @@
{
const size_t N = mObjects.size();
ALOGV("Killing %zu objects in manager %p", N, this);
- for (size_t i=0; i<N; i++) {
- const entry_t& e = mObjects.valueAt(i);
+ for (auto i : mObjects) {
+ const entry_t& e = i.second;
if (e.func != nullptr) {
- e.func(mObjects.keyAt(i), e.object, e.cleanupCookie);
+ e.func(i.first, e.object, e.cleanupCookie);
}
}
diff --git a/libs/binder/IMemory.cpp b/libs/binder/IMemory.cpp
index bd974b0..9c7ff97 100644
--- a/libs/binder/IMemory.cpp
+++ b/libs/binder/IMemory.cpp
@@ -31,9 +31,10 @@
#include <binder/Parcel.h>
#include <log/log.h>
-#include <utils/KeyedVector.h>
#include <utils/threads.h>
+#include <map>
+
#define VERBOSE 0
namespace android {
@@ -63,7 +64,7 @@
void free_heap(const wp<IBinder>& binder);
Mutex mHeapCacheLock; // Protects entire vector below.
- KeyedVector< wp<IBinder>, heap_info_t > mHeapCache;
+ std::map<wp<IBinder>, heap_info_t> mHeapCache;
// We do not use the copy-on-write capabilities of KeyedVector.
// TODO: Reimplemement based on standard C++ container?
};
@@ -434,9 +435,9 @@
sp<IMemoryHeap> HeapCache::find_heap(const sp<IBinder>& binder)
{
Mutex::Autolock _l(mHeapCacheLock);
- ssize_t i = mHeapCache.indexOfKey(binder);
- if (i>=0) {
- heap_info_t& info = mHeapCache.editValueAt(i);
+ auto i = mHeapCache.find(binder);
+ if (i != mHeapCache.end()) {
+ heap_info_t& info = i->second;
ALOGD_IF(VERBOSE,
"found binder=%p, heap=%p, size=%zu, fd=%d, count=%d",
binder.get(), info.heap.get(),
@@ -452,7 +453,7 @@
info.count = 1;
//ALOGD("adding binder=%p, heap=%p, count=%d",
// binder.get(), info.heap.get(), info.count);
- mHeapCache.add(binder, info);
+ mHeapCache.insert({binder, info});
return info.heap;
}
}
@@ -466,9 +467,9 @@
sp<IMemoryHeap> rel;
{
Mutex::Autolock _l(mHeapCacheLock);
- ssize_t i = mHeapCache.indexOfKey(binder);
- if (i>=0) {
- heap_info_t& info(mHeapCache.editValueAt(i));
+ auto i = mHeapCache.find(binder);
+ if (i != mHeapCache.end()) {
+ heap_info_t& info = i->second;
if (--info.count == 0) {
ALOGD_IF(VERBOSE,
"removing binder=%p, heap=%p, size=%zu, fd=%d, count=%d",
@@ -477,8 +478,8 @@
static_cast<BpMemoryHeap*>(info.heap.get())
->mHeapId.load(memory_order_relaxed),
info.count);
- rel = mHeapCache.valueAt(i).heap;
- mHeapCache.removeItemsAt(i);
+ rel = i->second.heap;
+ mHeapCache.erase(i);
}
} else {
ALOGE("free_heap binder=%p not found!!!", binder.unsafe_get());
@@ -490,23 +491,23 @@
{
sp<IMemoryHeap> realHeap;
Mutex::Autolock _l(mHeapCacheLock);
- ssize_t i = mHeapCache.indexOfKey(binder);
- if (i>=0) realHeap = mHeapCache.valueAt(i).heap;
- else realHeap = interface_cast<IMemoryHeap>(binder);
+ auto i = mHeapCache.find(binder);
+ if (i != mHeapCache.end())
+ realHeap = i->second.heap;
+ else
+ realHeap = interface_cast<IMemoryHeap>(binder);
return realHeap;
}
void HeapCache::dump_heaps()
{
Mutex::Autolock _l(mHeapCacheLock);
- int c = mHeapCache.size();
- for (int i=0 ; i<c ; i++) {
- const heap_info_t& info = mHeapCache.valueAt(i);
+ for (const auto& i : mHeapCache) {
+ const heap_info_t& info = i.second;
BpMemoryHeap const* h(static_cast<BpMemoryHeap const *>(info.heap.get()));
- ALOGD("hey=%p, heap=%p, count=%d, (fd=%d, base=%p, size=%zu)",
- mHeapCache.keyAt(i).unsafe_get(),
- info.heap.get(), info.count,
- h->mHeapId.load(memory_order_relaxed), h->mBase, h->mSize);
+ ALOGD("hey=%p, heap=%p, count=%d, (fd=%d, base=%p, size=%zu)", i.first.unsafe_get(),
+ info.heap.get(), info.count, h->mHeapId.load(memory_order_relaxed), h->mBase,
+ h->mSize);
}
}
diff --git a/libs/binder/IPCThreadState.cpp b/libs/binder/IPCThreadState.cpp
index 13f0a4c..f79075d 100644
--- a/libs/binder/IPCThreadState.cpp
+++ b/libs/binder/IPCThreadState.cpp
@@ -1199,7 +1199,8 @@
case BR_DECREFS:
refs = (RefBase::weakref_type*)mIn.readPointer();
- obj = (BBinder*)mIn.readPointer();
+ // NOLINTNEXTLINE(clang-analyzer-deadcode.DeadStores)
+ obj = (BBinder*)mIn.readPointer(); // consume
// NOTE: This assertion is not valid, because the object may no
// longer exist (thus the (BBinder*)cast above resulting in a different
// memory address).
@@ -1409,7 +1410,7 @@
uint32_t *async_received)
{
int ret = 0;
- binder_frozen_status_info info;
+ binder_frozen_status_info info = {};
info.pid = pid;
#if defined(__ANDROID__)
diff --git a/libs/binder/RpcSession.cpp b/libs/binder/RpcSession.cpp
index b84395e..e79cb86 100644
--- a/libs/binder/RpcSession.cpp
+++ b/libs/binder/RpcSession.cpp
@@ -152,8 +152,13 @@
}
status_t RpcSession::setupPreconnectedClient(unique_fd fd, std::function<unique_fd()>&& request) {
- return setupClient([&](const std::vector<uint8_t>& sessionId, bool incoming) -> status_t {
- // std::move'd from fd becomes -1 (!ok())
+ // Why passing raw fd? When fd is passed as reference, Clang analyzer sees that the variable
+ // `fd` is a moved-from object. To work-around the issue, unwrap the raw fd from the outer `fd`,
+ // pass the raw fd by value to the lambda, and then finally wrap it in unique_fd inside the
+ // lambda.
+ return setupClient([&, raw = fd.release()](const std::vector<uint8_t>& sessionId,
+ bool incoming) -> status_t {
+ unique_fd fd(raw);
if (!fd.ok()) {
fd = request();
if (!fd.ok()) return BAD_VALUE;
diff --git a/libs/binder/include/binder/BpBinder.h b/libs/binder/include/binder/BpBinder.h
index c0454b6..8deb2fe 100644
--- a/libs/binder/include/binder/BpBinder.h
+++ b/libs/binder/include/binder/BpBinder.h
@@ -17,10 +17,10 @@
#pragma once
#include <binder/IBinder.h>
-#include <utils/KeyedVector.h>
#include <utils/Mutex.h>
#include <utils/threads.h>
+#include <map>
#include <unordered_map>
#include <variant>
@@ -110,7 +110,7 @@
IBinder::object_cleanup_func func;
};
- KeyedVector<const void*, entry_t> mObjects;
+ std::map<const void*, entry_t> mObjects;
};
class PrivateAccessor {