diff options
author | Steven Moreland <smoreland@google.com> | 2019-04-26 14:51:24 -0700 |
---|---|---|
committer | Steven Moreland <smoreland@google.com> | 2019-09-06 16:55:04 -0700 |
commit | f0dd160cbe7c80204a2e361896f677a2bd12863f (patch) | |
tree | b20f1fe0f84780a1f6534d8ae0357a1879b3484e | |
parent | 48b66ef08b74440b1ca5da79d9ea9c6727a6d499 (diff) | |
download | libhidl-f0dd160cbe7c80204a2e361896f677a2bd12863f.tar.gz |
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
Merged-In: Ib52a16015b0393c104cd984376328cb0da888b03
-rw-r--r-- | Android.bp | 9 | ||||
-rw-r--r-- | base/HidlSupport.cpp | 13 | ||||
-rw-r--r-- | base/include/hidl/HidlSupport.h | 34 | ||||
-rw-r--r-- | test_main.cpp | 52 |
4 files changed, 81 insertions, 27 deletions
@@ -44,12 +44,15 @@ cc_test { shared_libs: [ "android.hidl.memory@1.0", "libbase", - "libhidlbase", - "liblog", "libutils", "libcutils", + "libvndksupport", + ], + static_libs: [ + "libhidlbase", + "libgtest", + "libgmock" ], - static_libs: ["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 @@ bool debuggable() { } } // 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 @@ void hidl_handle::freeHandle() { 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 89aa5a9..71448da 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 @@ struct hidl_handle { 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 @@ private: 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 @@ struct hidl_memory { 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 @@ -328,15 +336,12 @@ template<typename T> struct hidl_vec { using value_type = T; - 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. @@ -581,6 +586,7 @@ public: 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 7b6781a..083cee4 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> @@ -570,6 +575,51 @@ TEST_F(LibHidlTest, PreloadTest) { 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(); |