Avoid unaligned memory accesses in PBC deserialization

The pipeline binary cache (PBC) data format is as follows:
```
| Public Header | Private Header (20B) | Entry (24B) | Blob (n) | Entry (24B) | ...
```

Binary cache entry headers (`Entry` above) require the alignment of 8 bytes,
which is not guaranteed with this data format. Therefore we cannot use
`reinterpret_cast` to read cache entry headers when reading PBC blobs.

This bug was found with the undefined behavior sanitizer. Although we have not
seen this being a problem in real-world applications, we would like to
get this fixed so that we run the code under UBSan and find other issues
in the future.

Fixes: GPUOpen-Drivers#143.

icd/api/pipeline_binary_cache.cpp

@@ -136,6 +136,11 @@ PipelineBinaryCache* PipelineBinaryCache::Create(
         else if ((pInitData != nullptr) &&
                  (initDataSize > (sizeof(BinaryCacheEntry) + sizeof(PipelineBinaryCachePrivateHeader))))
         {
+            // The pipeline binary cache data format is as follows:
+            // ```
+            // | Public Header | Private Header (20B) | BinaryCacheEntry (24B) | Blob (n) | BinaryCacheEntry (24B) | ...
+            // ```
+            //
             const void* pBlob           = pInitData;
             size_t   blobSize           = initDataSize;
             constexpr size_t EntrySize  = sizeof(BinaryCacheEntry);
@@ -144,14 +149,18 @@ PipelineBinaryCache* PipelineBinaryCache::Create(
             blobSize     -= sizeof(PipelineBinaryCachePrivateHeader);
             while (blobSize > EntrySize)
             {
-                const BinaryCacheEntry* pEntry  = static_cast<const BinaryCacheEntry*>(pBlob);
-                const void*             pData   = Util::VoidPtrInc(pBlob, sizeof(BinaryCacheEntry));
-                const size_t entryAndDataSize   = pEntry->dataSize + sizeof(BinaryCacheEntry);
+                // `BinaryCacheEntry` headers require the alignment of 8 bytes, which is not guaranteed with this data
+                // format. Therefore we cannot use `reinterpret_cast` to read cache entry headers. We use memcpy to
+                // avoid unaligned memory accesses.
+                BinaryCacheEntry entry;
+                memcpy(&entry, pBlob, EntrySize);
+                const void*             pData   = Util::VoidPtrInc(pBlob, EntrySize);
+                const size_t entryAndDataSize   = entry.dataSize + EntrySize;
 
                 if (blobSize >= entryAndDataSize)
                 {
                     //add to cache
-                    Util::Result result = pObj->StorePipelineBinary(&pEntry->hashId, pEntry->dataSize, pData);
+                    Util::Result result = pObj->StorePipelineBinary(&entry.hashId, entry.dataSize, pData);
                     if (result != Util::Result::Success)
                     {
                         break;

tools/cache_creator/cache_info.cpp

@@ -50,7 +50,7 @@ llvm::Error createBlobError(llvm::MemoryBufferRef blob, llvm::Error err) {
 // @param vals : Values for the format specifier
 // @returns : Error annotated with the blob identifier
 template <typename... Ts>
-llvm::Error createBlobError(llvm::MemoryBufferRef blob, const char *format, const Ts &... vals) {
+llvm::Error createBlobError(llvm::MemoryBufferRef blob, const char *format, const Ts &...vals) {
   return createBlobError(blob, llvm::createStringError(std::errc::state_not_recoverable, format, vals...));
 }
 
@@ -170,10 +170,11 @@ CacheBlobInfo::readBinaryCacheEntriesInfo(llvm::SmallVectorImpl<BinaryCacheEntry
     }
 
     BinaryCacheEntryInfo currEntryInfo = {};
-    currEntryInfo.entryHeader = reinterpret_cast<const vk::BinaryCacheEntry *>(currData);
+    currEntryInfo.entryHeader = currData;
+    memcpy(&currEntryInfo.entryHeaderData, currData, EntrySize);
     currEntryInfo.idx = entryIdx;
 
-    const size_t currEntryBlobSize = currEntryInfo.entryHeader->dataSize;
+    const size_t currEntryBlobSize = currEntryInfo.entryHeaderData.dataSize;
     if (currData + EntrySize + currEntryBlobSize > blobEnd) {
       return createBlobError(m_cacheBlob, "Insufficient buffer size for cache entry content #%zu at offset %zu",
                              entryIdx, currEntryOffset);
@@ -222,12 +223,12 @@ llvm::raw_ostream &operator<<(llvm::raw_ostream &os, const BinaryCachePrivateHea
 
 llvm::raw_ostream &operator<<(llvm::raw_ostream &os, const BinaryCacheEntryInfo &info) {
   assert(info.entryHeader);
-  const vk::BinaryCacheEntry &header = *info.entryHeader;
+  const vk::BinaryCacheEntry &header = info.entryHeaderData;
 
   os << "\t*** Entry " << info.idx << " ***\n"
      << "\thash ID:\t\t"
-     << "0x" << llvm::format_hex_no_prefix(header.hashId.qwords[0], sizeof(uint64_t) * 2)
-     << " 0x" << llvm::format_hex_no_prefix(header.hashId.qwords[1], sizeof(uint64_t) * 2) << '\n'
+     << "0x" << llvm::format_hex_no_prefix(header.hashId.qwords[0], sizeof(uint64_t) * 2) << " 0x"
+     << llvm::format_hex_no_prefix(header.hashId.qwords[1], sizeof(uint64_t) * 2) << '\n'
      << "\tdata size:\t\t" << header.dataSize << "\n"
      << "\tcalculated MD5 sum:\t" << info.entryMD5Sum << "\n";
   return os;

tools/cache_creator/cache_info.h

@@ -79,7 +79,9 @@ using MD5DigestStr = llvm::SmallString<32>;
 // Represents printable information about a Pipeline Binary Cache entry, its location within the cache blob, and
 // calculated MD5 sum of the entry content.
 struct BinaryCacheEntryInfo {
-  const vk::BinaryCacheEntry *entryHeader;
+  // We do not store the header as `vk::BinaryCacheEntry *` because the address may not be properly aligned.
+  const uint8_t *entryHeader;
+  vk::BinaryCacheEntry entryHeaderData;
   size_t idx;
   llvm::ArrayRef<uint8_t> entryBlob;
   MD5DigestStr entryMD5Sum;

tools/cache_creator/unittests/cache_info_tests.cpp

@@ -28,6 +28,9 @@
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
 #include <cassert>
+#include <cstddef>
+#include <cstdint>
+#include <type_traits>
 
 namespace {
 
@@ -142,27 +145,47 @@ cc::MD5DigestStr calculateMD5Sum(llvm::ArrayRef<uint8_t> data) {
   return result.digest();
 }
 
+// =====================================================================================================================
+// Copies the contents of `value` to the buffer and returns the next position in the buffer. The caller must ensure that
+// the buffer sufficiently large to fit `value`.
+//
+// @param value : Value to add to the buffer.
+// @param [out] currData : Current position in the buffer. This does *not* have to be aligned to `alignof(T)`.
+// @returns : Next position in the buffer.
+template <typename T> uint8_t *appendRawData(T value, uint8_t *currData) {
+  static_assert(std::is_standard_layout<T>::value, "Incompatible type");
+  assert(currData);
+  memcpy(currData, &value, sizeof(T)); // Use memcpy instead of reinterpret_cast<T *> to avoid unaligned writes.
+  return currData + sizeof(T);
+}
+
 TEST(CacheInfoTest, ValidBlobOneEntry) {
   const size_t trailingSpace = 16;
   const size_t entrySize = sizeof(uint32_t);
   const size_t bufferSize = vk::VkPipelineCacheHeaderDataSize + trailingSpace +
                             sizeof(vk::PipelineBinaryCachePrivateHeader) + sizeof(vk::BinaryCacheEntry) + entrySize;
-  llvm::SmallVector<uint8_t> buffer(bufferSize);
+  std::vector<uint8_t> buffer(bufferSize);
   uint8_t *currData = buffer.data();
 
-  auto *publicHeader = new (currData) vk::PipelineCacheHeaderData();
-  publicHeader->headerLength = vk::VkPipelineCacheHeaderDataSize + trailingSpace;
-  currData += publicHeader->headerLength;
+  uint8_t *const publicHeader = currData;
+  vk::PipelineCacheHeaderData publicHeaderData = {};
+  publicHeaderData.headerLength = vk::VkPipelineCacheHeaderDataSize + trailingSpace;
+  currData = appendRawData(publicHeaderData, currData) + trailingSpace;
+
+  uint8_t *const privateHeader = currData;
+  currData = appendRawData(vk::PipelineBinaryCachePrivateHeader{}, currData);
 
-  auto *privateHeader = new (currData) vk::PipelineBinaryCachePrivateHeader();
-  currData += sizeof(vk::PipelineBinaryCachePrivateHeader);
+  uint8_t *const entryHeader = currData;
+  vk::BinaryCacheEntry cacheEntry = {};
+  cacheEntry.dataSize = entrySize;
+  currData = appendRawData(cacheEntry, currData);
 
-  auto *entryHeader = new (currData) vk::BinaryCacheEntry();
-  entryHeader->dataSize = entrySize;
-  currData += sizeof(vk::BinaryCacheEntry);
+  const uint32_t content = 42;
+  uint8_t *entryContent = currData;
+  currData = appendRawData(content, currData);
+  llvm::ArrayRef<uint8_t> entryBlob(entryContent, entrySize);
 
-  auto *entryContent = new (currData) uint32_t(42);
-  llvm::ArrayRef<uint8_t> entryBlob(currData, entrySize);
+  EXPECT_EQ(currData, buffer.data() + bufferSize);
 
   auto blobPtr = llvm::MemoryBuffer::getMemBuffer(llvm::toStringRef(buffer), "valid_one_entry", false);
   assert(blobPtr);
@@ -173,12 +196,13 @@ TEST(CacheInfoTest, ValidBlobOneEntry) {
   // These should all succeed as both headers can be fully parsed and there's one valid entry.
   auto publicHeaderInfoOrErr = blobInfoOrErr->readPublicVkHeaderInfo();
   ASSERT_THAT_EXPECTED(publicHeaderInfoOrErr, Succeeded());
-  EXPECT_EQ(publicHeaderInfoOrErr->publicHeader, publicHeader);
+  EXPECT_EQ(reinterpret_cast<const uint8_t *>(publicHeaderInfoOrErr->publicHeader), publicHeader);
+  EXPECT_EQ(publicHeaderInfoOrErr->publicHeader->headerLength, publicHeaderData.headerLength);
   EXPECT_EQ(publicHeaderInfoOrErr->trailingSpaceBeforePrivateBlob, trailingSpace);
 
   auto privateHeaderInfoOrErr = blobInfoOrErr->readBinaryCachePrivateHeaderInfo();
   ASSERT_THAT_EXPECTED(privateHeaderInfoOrErr, Succeeded());
-  EXPECT_EQ(privateHeaderInfoOrErr->privateHeader, privateHeader);
+  EXPECT_EQ(reinterpret_cast<const uint8_t *>(privateHeaderInfoOrErr->privateHeader), privateHeader);
   EXPECT_EQ(privateHeaderInfoOrErr->contentBlobSize, sizeof(vk::BinaryCacheEntry) + entrySize);
 
   llvm::SmallVector<cc::BinaryCacheEntryInfo, 1> entries;
@@ -188,6 +212,7 @@ TEST(CacheInfoTest, ValidBlobOneEntry) {
 
   cc::BinaryCacheEntryInfo &entry = entries.front();
   EXPECT_EQ(entry.entryHeader, entryHeader);
+  EXPECT_EQ(entry.entryHeaderData.dataSize, cacheEntry.dataSize);
   EXPECT_EQ(entry.idx, 0u);
   EXPECT_EQ(entry.entryBlob.data(), entryBlob.data());
   EXPECT_EQ(entry.entryBlob.size(), entryBlob.size());