Tests & stronger guarantees for hidl_* pads.
- Test that pads are zero (for future proofing, also ran tests
on original implementation and bugfix).
- Use -Wpadded so that compiler guarantees there aren't padding
bits hiding elsewhere (there aren't)
Bug: 131356202
Test: libhidl_test
(without fixes, with fixes, and with this CL)
Change-Id: Ib52a16015b0393c104cd984376328cb0da888b03
diff --git a/Android.bp b/Android.bp
index 97ed108..f83ada8 100644
--- a/Android.bp
+++ b/Android.bp
@@ -32,14 +32,18 @@
shared_libs: [
"android.hidl.memory@1.0",
"libbase",
- "libhidlbase",
- "libhidltransport",
"libhwbinder",
"liblog",
"libutils",
"libcutils",
+ "libvndksupport",
],
- static_libs: ["libgtest", "libgmock"],
+ static_libs: [
+ "libhidlbase",
+ "libhidltransport",
+ "libgtest",
+ "libgmock"
+ ],
cflags: [
"-O0",
diff --git a/base/HidlSupport.cpp b/base/HidlSupport.cpp
index f97f216..af805b9 100644
--- a/base/HidlSupport.cpp
+++ b/base/HidlSupport.cpp
@@ -35,10 +35,8 @@
}
} // namespace details
-hidl_handle::hidl_handle() {
- memset(this, 0, sizeof(*this));
- // mHandle = nullptr;
- // mOwnsHandle = false;
+hidl_handle::hidl_handle() : mHandle(nullptr), mOwnsHandle(false) {
+ memset(mPad, 0, sizeof(mPad));
}
hidl_handle::~hidl_handle() {
@@ -138,11 +136,8 @@
static const char *const kEmptyString = "";
-hidl_string::hidl_string() {
- memset(this, 0, sizeof(*this));
- // mSize is zero
- // mOwnsBuffer is false
- mBuffer = kEmptyString;
+hidl_string::hidl_string() : mBuffer(kEmptyString), mSize(0), mOwnsBuffer(false) {
+ memset(mPad, 0, sizeof(mPad));
}
hidl_string::~hidl_string() {
diff --git a/base/include/hidl/HidlSupport.h b/base/include/hidl/HidlSupport.h
index 7812099..112336f 100644
--- a/base/include/hidl/HidlSupport.h
+++ b/base/include/hidl/HidlSupport.h
@@ -20,18 +20,24 @@
#include <algorithm>
#include <array>
#include <iterator>
-#include <cutils/native_handle.h>
#include <hidl/HidlInternal.h>
-#include <hidl/Status.h>
#include <map>
#include <sstream>
#include <stddef.h>
#include <tuple>
#include <type_traits>
+#include <vector>
+
+// no requirements on types not used in scatter/gather
+// no requirements on other libraries
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wpadded"
+#include <cutils/native_handle.h>
+#include <hidl/Status.h>
#include <utils/Errors.h>
#include <utils/RefBase.h>
#include <utils/StrongPointer.h>
-#include <vector>
+#pragma clang diagnostic pop
namespace android {
@@ -123,8 +129,9 @@
private:
void freeHandle();
- details::hidl_pointer<const native_handle_t> mHandle __attribute__ ((aligned(8)));
- bool mOwnsHandle __attribute ((aligned(8)));
+ details::hidl_pointer<const native_handle_t> mHandle;
+ bool mOwnsHandle;
+ uint8_t mPad[7];
};
struct hidl_string {
@@ -174,6 +181,7 @@
details::hidl_pointer<const char> mBuffer;
uint32_t mSize; // NOT including the terminating '\0'.
bool mOwnsBuffer; // if true then mBuffer is a mutable char *
+ uint8_t mPad[3];
// copy from data with size. Assume that my memory is freed
// (through clear(), for example)
@@ -294,9 +302,9 @@
static const size_t kOffsetOfName;
private:
- hidl_handle mHandle __attribute__ ((aligned(8)));
- uint64_t mSize __attribute__ ((aligned(8)));
- hidl_string mName __attribute__ ((aligned(8)));
+ hidl_handle mHandle;
+ uint64_t mSize;
+ hidl_string mName;
};
// HidlMemory is a wrapper class to support sp<> for hidl_memory. It also
@@ -326,15 +334,12 @@
template<typename T>
struct hidl_vec {
- hidl_vec() {
+ hidl_vec() : mBuffer(nullptr), mSize(0), mOwnsBuffer(true) {
static_assert(hidl_vec<T>::kOffsetOfBuffer == 0, "wrong offset");
- memset(this, 0, sizeof(*this));
- // mSize is 0
- // mBuffer is nullptr
+ // mOwnsBuffer true to match original implementation
- // this is for consistency with the original implementation
- mOwnsBuffer = true;
+ memset(mPad, 0, sizeof(mPad));
}
// Note, does not initialize primitive types.
@@ -576,6 +581,7 @@
details::hidl_pointer<T> mBuffer;
uint32_t mSize;
bool mOwnsBuffer;
+ uint8_t mPad[3];
// copy from an array-like object, assuming my resources are freed.
template <typename Array>
diff --git a/test_main.cpp b/test_main.cpp
index 2b9f52c..9db076c 100644
--- a/test_main.cpp
+++ b/test_main.cpp
@@ -16,11 +16,16 @@
#define LOG_TAG "LibHidlTest"
+#pragma clang diagnostic push
+#pragma clang diagnostic fatal "-Wpadded"
+#include <hidl/HidlInternal.h>
+#include <hidl/HidlSupport.h>
+#pragma clang diagnostic pop
+
#include <android-base/logging.h>
#include <android/hidl/memory/1.0/IMemory.h>
#include <gmock/gmock.h>
#include <gtest/gtest.h>
-#include <hidl/HidlSupport.h>
#include <hidl/ServiceManagement.h>
#include <hidl/Status.h>
#include <hidl/TaskRunner.h>
@@ -488,6 +493,51 @@
EXPECT_TRUE(isLibraryOpen(kLib));
}
+template <typename T, size_t start, size_t end>
+static void assertZeroInRange(const T* t) {
+ static_assert(start < sizeof(T));
+ static_assert(end <= sizeof(T));
+
+ const uint8_t* ptr = reinterpret_cast<const uint8_t*>(t);
+
+ for (size_t i = start; i < end; i++) {
+ EXPECT_EQ(0, ptr[i]);
+ }
+}
+
+template <typename T, size_t start, size_t end>
+static void uninitTest() {
+ uint8_t buf[sizeof(T)];
+ memset(buf, 0xFF, sizeof(T));
+
+ T* type = new (buf) T;
+ assertZeroInRange<T, start, end>(type);
+ type->~T();
+}
+
+TEST_F(LibHidlTest, HidlVecUninit) {
+ using ::android::hardware::hidl_vec;
+ struct SomeType {};
+ static_assert(sizeof(hidl_vec<SomeType>) == 16);
+
+ // padding after mOwnsBuffer
+ uninitTest<hidl_vec<SomeType>, 13, 16>();
+}
+TEST_F(LibHidlTest, HidlHandleUninit) {
+ using ::android::hardware::hidl_handle;
+ static_assert(sizeof(hidl_handle) == 16);
+
+ // padding after mOwnsHandle
+ uninitTest<hidl_handle, 9, 16>();
+}
+TEST_F(LibHidlTest, HidlStringUninit) {
+ using ::android::hardware::hidl_string;
+ static_assert(sizeof(hidl_string) == 16);
+
+ // padding after mOwnsBuffer
+ uninitTest<hidl_string, 13, 16>();
+}
+
int main(int argc, char **argv) {
::testing::InitGoogleTest(&argc, argv);
return RUN_ALL_TESTS();
