From 5ff3abbc3151f28b990f14cff844b8724acdb12b Mon Sep 17 00:00:00 2001
From: Alexander Potapenko <glider@google.com>
Date: Fri, 20 Jul 2018 16:28:49 +0000
Subject: [PATCH] [MSan] run materializeChecks() before materializeStores()

When pointer checking is enabled, it's important that every pointer is
checked before its value is used.
For stores MSan used to generate code that calculates shadow/origin
addresses from a pointer before checking it.
For userspace this isn't a problem, because the shadow calculation code
is quite simple and compiler is able to move it after the check on -O2.
But for KMSAN getShadowOriginPtr() creates a runtime call, so we want the
check to be performed strictly before that call.

Swapping materializeChecks() and materializeStores() resolves the issue:
both functions insert code before the given IR location, so the new
insertion order guarantees that the code calculating shadow address is
between the address check and the memory access.

llvm-svn: 337571
---
 .../Transforms/Instrumentation/MemorySanitizer.cpp  | 13 ++++++-------
 .../MemorySanitizer/check_access_address.ll         |  3 +++
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp b/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
index 7828fcc432c2..40033fc7eb0b 100644
--- a/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
@@ -918,9 +918,6 @@ struct MemorySanitizerVisitor : public InstVisitor<MemorySanitizerVisitor> {
       StoreInst *NewSI = IRB.CreateAlignedStore(Shadow, ShadowPtr, Alignment);
       LLVM_DEBUG(dbgs() << "  STORE: " << *NewSI << "\n");
 
-      if (ClCheckAccessAddress)
-        insertShadowCheck(Addr, NewSI);
-
       if (SI->isAtomic())
         SI->setOrdering(addReleaseOrdering(SI->getOrdering()));
 
@@ -1024,13 +1021,13 @@ struct MemorySanitizerVisitor : public InstVisitor<MemorySanitizerVisitor> {
                                InstrumentationList.size() + StoreList.size() >
                                    (unsigned)ClInstrumentationWithCallThreshold;
 
-    // Delayed instrumentation of StoreInst.
-    // This may add new checks to be inserted later.
-    materializeStores(InstrumentWithCalls);
-
     // Insert shadow value checks.
     materializeChecks(InstrumentWithCalls);
 
+    // Delayed instrumentation of StoreInst.
+    // This may not add new address checks.
+    materializeStores(InstrumentWithCalls);
+
     return true;
   }
 
@@ -1490,6 +1487,8 @@ struct MemorySanitizerVisitor : public InstVisitor<MemorySanitizerVisitor> {
   /// Optionally, checks that the store address is fully defined.
   void visitStoreInst(StoreInst &I) {
     StoreList.push_back(&I);
+    if (ClCheckAccessAddress)
+      insertShadowCheck(I.getPointerOperand(), &I);
   }
 
   void handleCASOrRMW(Instruction &I) {
diff --git a/llvm/test/Instrumentation/MemorySanitizer/check_access_address.ll b/llvm/test/Instrumentation/MemorySanitizer/check_access_address.ll
index 38f29b71cdf1..21bb41256061 100644
--- a/llvm/test/Instrumentation/MemorySanitizer/check_access_address.ll
+++ b/llvm/test/Instrumentation/MemorySanitizer/check_access_address.ll
@@ -38,11 +38,14 @@ entry:
 
 ; CHECK-LABEL: @Store
 ; CHECK: load {{.*}} @__msan_param_tls
+; Shadow calculations must happen after the check.
+; CHECK-NOT: xor
 ; CHECK: icmp
 ; CHECK: br i1
 ; CHECK: <label>
 ; CHECK: call void @__msan_warning_noreturn
 ; CHECK: <label>
+; CHECK: xor
 ; CHECK: store
 ; CHECK: store i32 %x
 ; CHECK: ret void
-- 
GitLab