Don't initialize memory allocations in MSan builds
This allows detecting more potential uses of uninitialized memory in
MemorySanitizer (MSan) builds.
It is the application's responsibility to clear memory allocated by
vkAllocateMemory(), or guarantee that it will be written to before being
read. SwiftShader's internal uses of memory allocated by sw::allocate()
should be MSan error free at this point (explicitly initialized where
needed), and this change helps catch regressions.
Non-MSan builds are unaffected by this change, so that any errors that
are found in the near term don't pose a security risk. A subsequent
change will remove initialization for non-MSan builds as well.
Note that allocations made by the Reactor JIT-compiler will always
remain zero-initialized, since, contrary to the usage of sw::allocate(),
they are unrelated to Vulkan API allocations or other parts of the
graphics/compute pipeline which don't guarantee initialization, and are
not under the application's control.
Bug: b/140991626
Change-Id: I2c2a6c539ff48a5e7338d6c69e3e0c20a6ff7642
Reviewed-on: https://swiftshader-review.googlesource.com/c/SwiftShader/+/57629
Tested-by: Nicolas Capens <nicolascapens@google.com>
Reviewed-by: Alexis Hétu <sugoi@google.com>
diff --git a/src/Reactor/ExecutableMemory.cpp b/src/Reactor/ExecutableMemory.cpp
index bc01045..d35f3d5 100644
--- a/src/Reactor/ExecutableMemory.cpp
+++ b/src/Reactor/ExecutableMemory.cpp
@@ -49,12 +49,18 @@
#define STRINGIFY(x) #x
#define MACRO_STRINGIFY(x) STRINGIFY(x)
+// A Clang extension to determine compiler features.
+// We use it to detect Sanitizer builds (e.g. -fsanitize=memory).
+#ifndef __has_feature
+# define __has_feature(x) 0
+#endif
+
namespace rr {
namespace {
struct Allocation
{
- // size_t bytes;
+ // size_t bytes;
unsigned char *block;
};
@@ -87,7 +93,7 @@
aligned = (unsigned char *)((uintptr_t)(block + sizeof(Allocation) + alignment - 1) & -(intptr_t)alignment);
Allocation *allocation = (Allocation *)(aligned - sizeof(Allocation));
- // allocation->bytes = bytes;
+ // allocation->bytes = bytes;
allocation->block = block;
}
@@ -231,7 +237,10 @@
{
void *memory = allocateRaw(bytes, alignment);
- if(memory)
+ // Zero-initialize the memory, for security reasons.
+ // MemorySanitizer builds skip this so that we can detect when we
+ // inadvertently rely on this, which would indicate a bug.
+ if(memory && !__has_feature(memory_sanitizer))
{
memset(memory, 0, bytes);
}
diff --git a/src/System/Memory.cpp b/src/System/Memory.cpp
index 773b362..6c0d300 100644
--- a/src/System/Memory.cpp
+++ b/src/System/Memory.cpp
@@ -40,35 +40,22 @@
# define __x86__
#endif
+// A Clang extension to determine compiler features.
+// We use it to detect Sanitizer builds (e.g. -fsanitize=memory).
+#ifndef __has_feature
+# define __has_feature(x) 0
+#endif
+
namespace sw {
namespace {
struct Allocation
{
- // size_t bytes;
+ // size_t bytes;
unsigned char *block;
};
-void *allocateRaw(size_t bytes, size_t alignment)
-{
- ASSERT((alignment & (alignment - 1)) == 0); // Power of 2 alignment.
-
- unsigned char *block = (unsigned char *)malloc(bytes + sizeof(Allocation) + alignment);
- unsigned char *aligned = nullptr;
-
- if(block)
- {
- aligned = (unsigned char *)((uintptr_t)(block + sizeof(Allocation) + alignment - 1) & -(intptr_t)alignment);
- Allocation *allocation = (Allocation *)(aligned - sizeof(Allocation));
-
- // allocation->bytes = bytes;
- allocation->block = block;
- }
-
- return aligned;
-}
-
} // anonymous namespace
size_t memoryPageSize()
@@ -88,14 +75,28 @@
void *allocate(size_t bytes, size_t alignment)
{
- void *memory = allocateRaw(bytes, alignment);
+ ASSERT((alignment & (alignment - 1)) == 0); // Power of 2 alignment.
- if(memory)
+ size_t size = bytes + sizeof(Allocation) + alignment;
+ unsigned char *block = (unsigned char *)malloc(size);
+ unsigned char *aligned = nullptr;
+
+ if(block)
{
- memset(memory, 0, bytes);
+ // TODO(b/140991626): Never initialize the allocated memory.
+ if(!__has_feature(memory_sanitizer))
+ {
+ memset(block, 0, size);
+ }
+
+ aligned = (unsigned char *)((uintptr_t)(block + sizeof(Allocation) + alignment - 1) & -(intptr_t)alignment);
+ Allocation *allocation = (Allocation *)(aligned - sizeof(Allocation));
+
+ // allocation->bytes = bytes;
+ allocation->block = block;
}
- return memory;
+ return aligned;
}
void deallocate(void *memory)
diff --git a/tests/VulkanUnitTests/ComputeTests.cpp b/tests/VulkanUnitTests/ComputeTests.cpp
index 9d94987..d84df6f 100644
--- a/tests/VulkanUnitTests/ComputeTests.cpp
+++ b/tests/VulkanUnitTests/ComputeTests.cpp
@@ -185,6 +185,8 @@
uint32_t *buffers;
VK_ASSERT(device->MapMemory(memory, 0, buffersSize, 0, (void **)&buffers));
+ memset(buffers, 0, buffersSize);
+
buffers[magic0Offset] = magic0;
buffers[magic1Offset] = magic1;
buffers[magic2Offset] = magic2;
