Do not destruct static maps.
In case they are deleted while some threads are using them (this
happens sometimes depending on how these libraries are loaded and how
they are being used).
Noting that b/69122224 is filed to move these into functions.
Bug: 129726097
Test: HIDL's run_all_device_tests.sh
Test: no longer see error where it repro'd before:
FORTIFY: pthread_mutex_lock called on a destroyed mutex
Change-Id: I66eb9aa24e31d7fc652f0426361c17600b4716ec
diff --git a/transport/HidlBinderSupport.cpp b/transport/HidlBinderSupport.cpp
index caf7cf8..62b1755 100644
--- a/transport/HidlBinderSupport.cpp
+++ b/transport/HidlBinderSupport.cpp
@@ -256,16 +256,16 @@
}
// for get + set
- std::unique_lock<std::mutex> _lock = details::gBnMap.lock();
+ std::unique_lock<std::mutex> _lock = details::gBnMap->lock();
- wp<BHwBinder> wBnObj = details::gBnMap.getLocked(ifacePtr, nullptr);
+ wp<BHwBinder> wBnObj = details::gBnMap->getLocked(ifacePtr, nullptr);
sp<IBinder> sBnObj = wBnObj.promote();
if (sBnObj == nullptr) {
auto func = details::getBnConstructorMap().get(descriptor, nullptr);
if (!func) {
// TODO(b/69122224): remove this static variable when prebuilts updated
- func = details::gBnConstructorMap.get(descriptor, nullptr);
+ func = details::gBnConstructorMap->get(descriptor, nullptr);
}
LOG_ALWAYS_FATAL_IF(func == nullptr, "%s gBnConstructorMap returned null for %s", __func__,
descriptor.c_str());
@@ -274,7 +274,7 @@
LOG_ALWAYS_FATAL_IF(sBnObj == nullptr, "%s Bn constructor function returned null for %s",
__func__, descriptor.c_str());
- details::gBnMap.setLocked(ifacePtr, static_cast<BHwBinder*>(sBnObj.get()));
+ details::gBnMap->setLocked(ifacePtr, static_cast<BHwBinder*>(sBnObj.get()));
}
return sBnObj;
diff --git a/transport/HidlPassthroughSupport.cpp b/transport/HidlPassthroughSupport.cpp
index ff68a1e..bc67656 100644
--- a/transport/HidlPassthroughSupport.cpp
+++ b/transport/HidlPassthroughSupport.cpp
@@ -31,7 +31,7 @@
auto func = getBsConstructorMap().get(descriptor, nullptr);
if (!func) {
// TODO(b/69122224): remove this when prebuilts don't reference it
- func = gBsConstructorMap.get(descriptor, nullptr);
+ func = gBsConstructorMap->get(descriptor, nullptr);
}
if (func) {
return func(static_cast<void*>(iface.get()));
diff --git a/transport/HidlTransportSupport.cpp b/transport/HidlTransportSupport.cpp
index 16f47c6..846b166 100644
--- a/transport/HidlTransportSupport.cpp
+++ b/transport/HidlTransportSupport.cpp
@@ -88,9 +88,9 @@
// Due to ABI considerations, IBase cannot have a destructor to clean this up.
// So, because this API is so infrequently used, (expected to be usually only
// one time for a process, but it can be more), we are cleaning it up here.
- std::unique_lock<std::mutex> lock = details::gServicePrioMap.lock();
- pruneMapLocked(details::gServicePrioMap);
- details::gServicePrioMap.setLocked(service, {policy, priority});
+ std::unique_lock<std::mutex> lock = details::gServicePrioMap->lock();
+ pruneMapLocked(details::gServicePrioMap.get());
+ details::gServicePrioMap->setLocked(service, {policy, priority});
return true;
}
@@ -104,9 +104,9 @@
// Due to ABI considerations, IBase cannot have a destructor to clean this up.
// So, because this API is so infrequently used, (expected to be usually only
// one time for a process, but it can be more), we are cleaning it up here.
- std::unique_lock<std::mutex> lock = details::gServiceSidMap.lock();
- pruneMapLocked(details::gServiceSidMap);
- details::gServiceSidMap.setLocked(service, requesting);
+ std::unique_lock<std::mutex> lock = details::gServiceSidMap->lock();
+ pruneMapLocked(details::gServiceSidMap.get());
+ details::gServiceSidMap->setLocked(service, requesting);
return true;
}
diff --git a/transport/InternalStatic.h b/transport/InternalStatic.h
index b0fefa9..1dfaae4 100644
--- a/transport/InternalStatic.h
+++ b/transport/InternalStatic.h
@@ -26,10 +26,12 @@
namespace hardware {
namespace details {
+// TODO(b/69122224): remove this
// deprecated; use getBnConstructorMap instead.
-extern BnConstructorMap gBnConstructorMap;
+extern DoNotDestruct<BnConstructorMap> gBnConstructorMap;
+// TODO(b/69122224): remove this
// deprecated; use getBsConstructorMap instead.
-extern BsConstructorMap gBsConstructorMap;
+extern DoNotDestruct<BsConstructorMap> gBsConstructorMap;
} // namespace details
} // namespace hardware
diff --git a/transport/Static.cpp b/transport/Static.cpp
index 0bbd48d..af16e8f 100644
--- a/transport/Static.cpp
+++ b/transport/Static.cpp
@@ -28,27 +28,28 @@
namespace details {
// Deprecated; kept for ABI compatibility. Use getBnConstructorMap.
-BnConstructorMap gBnConstructorMap{};
+DoNotDestruct<BnConstructorMap> gBnConstructorMap{};
-ConcurrentMap<const ::android::hidl::base::V1_0::IBase*, wp<::android::hardware::BHwBinder>>
- gBnMap{};
+DoNotDestruct<ConcurrentMap<const ::android::hidl::base::V1_0::IBase*,
+ wp<::android::hardware::BHwBinder>>>
+ gBnMap{};
// TODO(b/122472540): replace with single, hidden map
-ConcurrentMap<wp<::android::hidl::base::V1_0::IBase>, SchedPrio> gServicePrioMap{};
-ConcurrentMap<wp<::android::hidl::base::V1_0::IBase>, bool> gServiceSidMap{};
+DoNotDestruct<ConcurrentMap<wp<::android::hidl::base::V1_0::IBase>, SchedPrio>> gServicePrioMap{};
+DoNotDestruct<ConcurrentMap<wp<::android::hidl::base::V1_0::IBase>, bool>> gServiceSidMap{};
// Deprecated; kept for ABI compatibility. Use getBsConstructorMap.
-BsConstructorMap gBsConstructorMap{};
+DoNotDestruct<BsConstructorMap> gBsConstructorMap{};
// For static executables, it is not guaranteed that gBnConstructorMap are initialized before
// used in HAL definition libraries.
BnConstructorMap& getBnConstructorMap() {
- static BnConstructorMap map{};
+ static BnConstructorMap& map = *new BnConstructorMap();
return map;
}
BsConstructorMap& getBsConstructorMap() {
- static BsConstructorMap map{};
+ static BsConstructorMap& map = *new BsConstructorMap();
return map;
}
diff --git a/transport/include/hidl/ConcurrentMap.h b/transport/include/hidl/ConcurrentMap.h
index 1b06dfd..329752c 100644
--- a/transport/include/hidl/ConcurrentMap.h
+++ b/transport/include/hidl/ConcurrentMap.h
@@ -90,6 +90,23 @@
std::map<K, V> mMap;
};
+namespace details {
+
+// TODO(b/69122224): remove this type and usages of it
+// DO NOT ADD USAGES
+template <typename T>
+class DoNotDestruct {
+ public:
+ DoNotDestruct() { new (buffer) T(); }
+ T& get() { return *reinterpret_cast<T*>(buffer); }
+ T* operator->() { return reinterpret_cast<T*>(buffer); }
+
+ private:
+ alignas(T) char buffer[sizeof(T)];
+};
+
+} // namespace details
+
} // namespace hardware
} // namespace android
diff --git a/transport/include/hidl/Static.h b/transport/include/hidl/Static.h
index cc711b7..be74bae 100644
--- a/transport/include/hidl/Static.h
+++ b/transport/include/hidl/Static.h
@@ -37,12 +37,17 @@
int prio;
};
-extern ConcurrentMap<wp<::android::hidl::base::V1_0::IBase>, SchedPrio> gServicePrioMap;
-extern ConcurrentMap<wp<::android::hidl::base::V1_0::IBase>, bool> gServiceSidMap;
+// TODO(b/69122224): remove this
+extern DoNotDestruct<ConcurrentMap<wp<::android::hidl::base::V1_0::IBase>, SchedPrio>>
+ gServicePrioMap;
+// TODO(b/69122224): remove this
+extern DoNotDestruct<ConcurrentMap<wp<::android::hidl::base::V1_0::IBase>, bool>> gServiceSidMap;
+// TODO(b/69122224): remove this
// For HidlBinderSupport and autogenerated code
-extern ConcurrentMap<const ::android::hidl::base::V1_0::IBase*, wp<::android::hardware::BHwBinder>>
- gBnMap;
+extern DoNotDestruct<ConcurrentMap<const ::android::hidl::base::V1_0::IBase*,
+ wp<::android::hardware::BHwBinder>>>
+ gBnMap;
using BnConstructorMap = ConcurrentMap<std::string, std::function<sp<IBinder>(void*)>>;
// For HidlBinderSupport and autogenerated code