[vm/msan] Fix MemCopyInstr instrumentation

The MemoryCopyInstr on x64 in MSAN mode when forward copying was
unpoisoning at the wrong pointer. The `rep mov` changes the
destination register.

Now we unpoison _before_ the `rep mov` if it's a forward copy, and
_after_ the `rep mov` for backwards copies. This way the destination
register contains the start of the memory in both cases.

TEST=tests/ffi/msan_test.dart

b/266213262
Change-Id: I37688802e63797f650a8242b6ba8b884813ebbd0
Cq-Include-Trybots: luci.dart.try:vm-msan-linux-release-x64-try,vm-aot-msan-linux-release-x64-try,vm-linux-debug-ia32-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/361420
Reviewed-by: Tess Strickland <sstrickl@google.com>
Commit-Queue: Daco Harkes <dacoharkes@google.com>

runtime/bin/ffi_test/ffi_test_functions.cc

@@ -1331,4 +1331,9 @@ DART_EXPORT int32_t CallTwoPointerIntFunction(int32_t (*fn)(void*, void*),
   return fn(a, b);
 }
 
+DART_EXPORT char TakeString(char* my_string) {
+  std::cout << "TakeString(" << my_string << ")\n";
+  return my_string[4];
+}
+
 }  // namespace dart

runtime/vm/compiler/assembler/assembler_base.cc

@@ -378,6 +378,8 @@ intptr_t AssemblerBase::InsertAlignedRelocation(BSS::Relocation reloc) {
 }
 
 void AssemblerBase::MsanUnpoison(Register base, intptr_t length_in_bytes) {
+  Comment("MsanUnpoison base %s length_in_bytes %" Pd,
+          RegisterNames::RegisterName(base), length_in_bytes);
   LeafRuntimeScope rt(static_cast<Assembler*>(this), /*frame_size=*/0,
                       /*preserve_registers=*/true);
   MoveRegister(CallingConventions::ArgumentRegisters[0], base);
@@ -386,6 +388,9 @@ void AssemblerBase::MsanUnpoison(Register base, intptr_t length_in_bytes) {
 }
 
 void AssemblerBase::MsanUnpoison(Register base, Register length_in_bytes) {
+  Comment("MsanUnpoison base %s length_in_bytes %s",
+          RegisterNames::RegisterName(base),
+          RegisterNames::RegisterName(length_in_bytes));
   LeafRuntimeScope rt(static_cast<Assembler*>(this), /*frame_size=*/0,
                       /*preserve_registers=*/true);
   const Register a0 = CallingConventions::ArgumentRegisters[0];

runtime/vm/compiler/backend/il_x64.cc

@@ -250,8 +250,16 @@ void MemoryCopyInstr::EmitLoopCopy(FlowGraphCompiler* compiler,
     __ std();
   }
 #if defined(USING_MEMORY_SANITIZER)
-  // The `rep` instruction sets `length_reg` to 0.
+  // For reversed, do the `rep` first. It sets `dest_reg` to the start again.
+  // For forward, do the unpoisining first, before `dest_reg` is modified.
   __ movq(TMP, length_reg);
+  if (mov_size != 1) {
+    // Unpoison takes the length in bytes.
+    __ MulImmediate(TMP, mov_size);
+  }
+  if (!reversed) {
+    __ MsanUnpoison(dest_reg, TMP);
+  }
 #endif
   switch (mov_size) {
     case 1:
@@ -274,8 +282,9 @@ void MemoryCopyInstr::EmitLoopCopy(FlowGraphCompiler* compiler,
   }
 
 #if defined(USING_MEMORY_SANITIZER)
-  __ MulImmediate(TMP, mov_size);
-  __ MsanUnpoison(dest_reg, TMP);
+  if (reversed) {
+    __ MsanUnpoison(dest_reg, TMP);
+  }
 #endif
 }
 

tests/ffi/msan_test.dart

@@ -0,0 +1,57 @@
+// Copyright (c) 2024, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// SharedObjects=ffi_test_functions
+
+import 'dart:ffi';
+import 'dart:typed_data';
+
+import 'package:expect/expect.dart';
+import 'package:ffi/ffi.dart';
+
+import 'ffi_test_helpers.dart';
+
+void main() {
+  testInitMemoryInDart();
+  testInitMemoryInDart16();
+  testInitStringInDart();
+}
+
+void testInitMemoryInDart() {
+  final units = Uint8List.fromList([109, 121, 83, 116, 114, 105, 110, 103, 0]);
+  final pointer = malloc<Uint8>(units.length);
+  pointer.asTypedList(units.length).setAll(0, units);
+  print(pointer);
+  final result = takeString(pointer.cast());
+  Expect.equals(114, result);
+  malloc.free(pointer);
+}
+
+void testInitMemoryInDart16() {
+  final units = Uint16List.fromList([
+    109 + 121 * 256,
+    83 + 116 * 256,
+    114 + 105 * 256,
+    110 + 103 * 256,
+    0,
+  ]);
+  final Pointer<Uint16> pointer = malloc<Uint16>(units.length);
+  pointer.asTypedList(units.length).setAll(0, units);
+  print(pointer);
+  final result = takeString(pointer.cast());
+  Expect.equals(114, result);
+  malloc.free(pointer);
+}
+
+void testInitStringInDart() {
+  final cString = 'myString'.toNativeUtf8();
+  final result = takeString(cString);
+  Expect.equals(114, result);
+  malloc.free(cString);
+}
+
+final takeString = ffiTestFunctions
+    .lookupFunction<Char Function(Pointer<Utf8>), int Function(Pointer<Utf8>)>(
+  'TakeString',
+);