[MSan] Ensure argument shadow initialized on memcpy

[nikic]

Summary

We need to explicitly query the shadow here, because it is lazily initialized for arguments. Without opaque pointers this used to mostly work out, because there would be a bitcast to i8* present, and that would query the argument shadow.

[vitalybuka]

I am not sure how this can help, I will try later today.
As I see it it just calculates the shadow address and these instructions will be removed as unused?

[nikic]

In D123602#3445885, @vitalybuka wrote:

I am not sure how this can help, I will try later today.
As I see it it just calculates the shadow address and these instructions will be removed as unused?

Shadow memory for arguments is lazily initialized. If we never call getShadow() for an argument, then we'll leave it uninitialized. In most cases this happens naturally, but for memcpy/memmove we call a builtin that will access the shadow internally, so we never perform the getShadow() call and never initialize the shadow.

[vitalybuka]

In D123602#3445899, @nikic wrote:

In D123602#3445885, @vitalybuka wrote:

I am not sure how this can help, I will try later today.
As I see it it just calculates the shadow address and these instructions will be removed as unused?

Shadow memory for arguments is lazily initialized. If we never call getShadow() for an argument, then we'll leave it uninitialized. In most cases this happens naturally, but for memcpy/memmove we call a builtin that will access the shadow internally, so we never perform the getShadow() call and never initialize the shadow.

I see. I suspect it does not work if instead of llvm.memcpy we have "call void @foo(i8* %p)" and "call void @foo_noundef(i8* noundef %p)"?

Most likely it would, as visitCallBase has it on all code paths, but could you add this into the test?

[eugenis]

Could you clarify that the problem is specifically with byval argument in the patch description?

Also, I'm wondering if we should make this copy explicit, perhaps by checking if the argument has any uses in the function body. I'm not 100% convinced that this is the only place where we forget to copy the shadow.

This change LGTM as a quick fix.

[vitalybuka]

Works locally, so I landed it to fix the bot.
I will address my and @eugenis suggestions separately.

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp

#endif
   /// If we don't instrument it and it gets inlined,
   /// our interceptor will not kick in and we will lose the memmove.
   /// If we instrument the call here, but it does not get inlined,
   /// we will memove the shadow twice: which is bad in case
   /// of overlapping regions. So, we simply lower the intrinsic to a call.
   ///
   /// Similar situation exists for memcpy and memset.
   void visitMemMoveInst(MemMoveInst &I) {
+    getShadow(I.getArgOperand(1)); // Ensure shadow initialized
     IRBuilder<> IRB(&I);
     IRB.CreateCall(
         MS.MemmoveFn,
         {IRB.CreatePointerCast(I.getArgOperand(0), IRB.getInt8PtrTy()),
          IRB.CreatePointerCast(I.getArgOperand(1), IRB.getInt8PtrTy()),
          IRB.CreateIntCast(I.getArgOperand(2), MS.IntptrTy, false)});
     I.eraseFromParent();
   }
 
   // Similar to memmove: avoid copying shadow twice.
   // This is somewhat unfortunate as it may slowdown small constant memcpys.
   // FIXME: consider doing manual inline for small constant sizes and proper
   // alignment.
   void visitMemCpyInst(MemCpyInst &I) {
+    getShadow(I.getArgOperand(1)); // Ensure shadow initialized
     IRBuilder<> IRB(&I);
     IRB.CreateCall(
         MS.MemcpyFn,
         {IRB.CreatePointerCast(I.getArgOperand(0), IRB.getInt8PtrTy()),
          IRB.CreatePointerCast(I.getArgOperand(1), IRB.getInt8PtrTy()),
          IRB.CreateIntCast(I.getArgOperand(2), MS.IntptrTy, false)});
     I.eraseFromParent();
   }

llvm/test/Instrumentation/MemorySanitizer/opaque-ptr.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S -passes=msan < %s | FileCheck %s
+
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @test_memcpy(ptr %p, ptr byval(i32) %p2) sanitize_memory {
+; CHECK-LABEL: @test_memcpy(
+; CHECK-NEXT:    [[TMP1:%.*]] = ptrtoint ptr [[P2:%.*]] to i64
+; CHECK-NEXT:    [[TMP2:%.*]] = xor i64 [[TMP1]], 87960930222080
+; CHECK-NEXT:    [[TMP3:%.*]] = inttoptr i64 [[TMP2]] to ptr
+; CHECK-NEXT:    call void @llvm.memcpy.p0.p0.i64(ptr align 4 [[TMP3]], ptr align 4 inttoptr (i64 add (i64 ptrtoint (ptr @__msan_param_tls to i64), i64 8) to ptr), i64 4, i1 false)
+; CHECK-NEXT:    call void @llvm.donothing()
+; CHECK-NEXT:    [[TMP4:%.*]] = call ptr @__msan_memcpy(ptr [[P:%.*]], ptr [[P2]], i64 4)
+; CHECK-NEXT:    ret void
+;
+  call void @llvm.memcpy.p0.p0.i64(i8* %p, i8* %p2, i64 4, i1 false)
+  ret void
+}
+
+define void @test_memmove(ptr %p, ptr byval(i32) %p2) sanitize_memory {
+; CHECK-LABEL: @test_memmove(
+; CHECK-NEXT:    [[TMP1:%.*]] = ptrtoint ptr [[P2:%.*]] to i64
+; CHECK-NEXT:    [[TMP2:%.*]] = xor i64 [[TMP1]], 87960930222080
+; CHECK-NEXT:    [[TMP3:%.*]] = inttoptr i64 [[TMP2]] to ptr
+; CHECK-NEXT:    call void @llvm.memcpy.p0.p0.i64(ptr align 4 [[TMP3]], ptr align 4 inttoptr (i64 add (i64 ptrtoint (ptr @__msan_param_tls to i64), i64 8) to ptr), i64 4, i1 false)
+; CHECK-NEXT:    call void @llvm.donothing()
+; CHECK-NEXT:    [[TMP4:%.*]] = call ptr @__msan_memmove(ptr [[P:%.*]], ptr [[P2]], i64 4)
+; CHECK-NEXT:    ret void
+;
+  call void @llvm.memmove.p0.p0.i64(i8* %p, i8* %p2, i64 4, i1 false)
+  ret void
+}
+
+declare void @llvm.memcpy.p0.p0.i64(i8*, i8*, i64, i1)
+declare void @llvm.memmove.p0.p0.i64(i8*, i8*, i64, i1)