[compiler-rt][asan] Fix overlaping parameters for memmove/memcpy on windows.

[etienneb]

Summary

On windows, memmove and memcpy may be the same functions (on 64-bits).

-- f:\dd\vctools\crt\vcruntime\src\string\amd64\memcpy.asm --------------------

        OPTION PROLOGUE:NONE, EPILOGUE:NONE

        memmove = memcpy
        mov     r11, rcx                ; save destination address

This is causing ASAN to report overlaping parameters when instrumenting chromium.

D:\src\chromium\src>out\asan64\chrome.exe --no-sandbox
[8956:6208:1121/162511:ERROR:entry.cc(167)] Entry::Deserialize: dictionary has no interface_provider_specs key
[8956:11560:1121/162511:ERROR:external_registry_loader_win.cc(130)] Missing value path for key Software\Google\Chrome\Ex
tensions\doeiiacdhfmpdeckdaifnjaemmkkdlkf.
=================================================================
==5132==ERROR: AddressSanitizer: memcpy-param-overlap: memory ranges [0x000000237ee8,0x000000237eea) and [0x000000237ee9
, 0x000000237eeb) overlap

The error triggered on chromium:

Child-SP          RetAddr           Call Site
00000000`00166520 00000001`400a4886 chrome!__asan::ReportStringFunctionMemoryRangesOverlap+0x23 [d:\src\llvm\llvm\projects\compiler-rt\lib\asan\asan_report.cc @ 305]
*** WARNING: Unable to verify checksum for D:\src\chromium\src\out\asan64dynamic\libglesv2.dll
00000000`001672a0 000007fe`e1859607 chrome!__asan_wrap_memcpy+0xf6 [d:\src\llvm\llvm\projects\compiler-rt\lib\asan\asan_interceptors.cc @ 458]
00000000`00167b30 000007fe`e184bcbc libglesv2!__acrt_fp_strflt_to_string+0xb7 [d:\th\minkernel\crts\ucrt\src\appcrt\convert\_fptostr.cpp @ 86]
(Inline Function) --------`-------- libglesv2!fp_format_f+0x57 [d:\th\minkernel\crts\ucrt\src\appcrt\convert\cvt.cpp @ 578]
00000000`00167b60 000007fe`e182e2a2 libglesv2!__acrt_fp_format+0x180 [d:\th\minkernel\crts\ucrt\src\appcrt\convert\cvt.cpp @ 722]
00000000`00167bf0 000007fe`e182ce80 libglesv2!__crt_stdio_output::output_processor<char,__crt_stdio_output::stream_output_adapter<char>,__crt_stdio_output::format_validation_

This bug is similar to: https://llvm.org/bugs/show_bug.cgi?id=16362

[etienneb]

lib/asan/asan_interceptors.cc

413

This line seems incorrect for iOS.
We should never use 'PLATFORM_HAS_DIFFERENT_MEMCPY_AND_MEMMOVE' within a "#if/def" and only a "if" statement.

// Platform-specific options.
#if SANITIZER_MAC
bool PlatformHasDifferentMemcpyAndMemmove();
# define PLATFORM_HAS_DIFFERENT_MEMCPY_AND_MEMMOVE \
    (PlatformHasDifferentMemcpyAndMemmove())
#elif SANITIZER_WINDOWS64
# define PLATFORM_HAS_DIFFERENT_MEMCPY_AND_MEMMOVE false
#else
# define PLATFORM_HAS_DIFFERENT_MEMCPY_AND_MEMMOVE true
#endif  // SANITIZER_MAC

[etienneb]

fix incorrect defines

[rnk]

lgtm

It seems like on Mac, memcpy == memmove since 10.6, which is so old, I would support a follow-up change to remove this logic:

bool PlatformHasDifferentMemcpyAndMemmove() {
  // On OS X 10.7 memcpy() and memmove() are both resolved
  // into memmove$VARIANT$sse42.
  // See also https://github.com/google/sanitizers/issues/34.
  // TODO(glider): need to check dynamically that memcpy() and memmove() are
  // actually the same function.
  return GetMacosVersion() == MACOS_VERSION_SNOW_LEOPARD;
}

[filcab]

Which function is that code calling, though? memcpy or memmove?

Shouldn't the alias be taken care of by this? (in asan_interceptors.cc)

if (PLATFORM_HAS_DIFFERENT_MEMCPY_AND_MEMMOVE) {
  // In asan, REAL(memmove) is not used, but it is used in msan.
  ASAN_INTERCEPT_FUNC(memcpy);
} else {
  ASSIGN_REAL(memcpy, memmove);
}

[filcab]

lib/asan/asan_interceptors.cc

416

I have a problem with this. It seems like it's hiding problems.

If the runtime called __asan_memcpy, it is because we actually had a call to memcpy, so we should check for overlap properly. This code is turning that off.
If memcpy and memmove are defined to the same, we shouldn't be erroring when memmove is called, though. But I don't see how calls to __asan_memcpy/__asan_memmove get converted to be the same function, since that's an ASan function, not the libc one.

456–457

This part looks ok, of course.

[filcab]

P.S: Please add a test for Win64 that shows this problem and that it's fixed.

[rnk]

lib/asan/asan_interceptors.cc

416

In practice, we found, as Apple did, that on platforms where memcpy aliases memmove, it is not feasible to intercept them separately. If you trace through the existing logic, you'll see that we're already suppressing this error in the same way on Mac.

[filcab]

Sorry for the duplication between email and phab, but I want to be sure this is included in the review page on phabricator and it didn't pick up the email.

lib/asan/asan_interceptors.cc

416

But only for the interceptors. __asan_memcpy should only be called if instrumentMemIntrinsic added it, in AddressSanitizer.cpp. I don't think macOS is doing anything special there (if it were, this patch wouldn't even have the section where I posted the comment, as ASan would be handling it already)

[rnk]

lib/asan/asan_interceptors.cc

416

Oh, right. Let's not change __asan_memcpy, then.

[zaks.anna]

lib/asan/asan_interceptors.cc

456–457

Why do we need "!SANITIZER_MAC"? Wouldn't just calling PLATFORM_HAS_DIFFERENT_MEMCPY_AND_MEMMOVE work on the Mac? It's defined as :
bool PlatformHasDifferentMemcpyAndMemmove() {

// On OS X 10.7 memcpy() and memmove() are both resolved
// into memmove$VARIANT$sse42.
// See also https://github.com/google/sanitizers/issues/34.
// TODO(glider): need to check dynamically that memcpy() and memmove() are
// actually the same function.
return GetMacosVersion() == MACOS_VERSION_SNOW_LEOPARD;

}

[filcab]

lib/asan/asan_interceptors.cc

456–457

Indeed, a regular

if (PLATFORM_HAS_DIFFERENT_MEMCPY_AND_MEMMOVE)
  ASAN_MEMCPY_IMPL(ctx, to, from, size);
else
  // Keep the comments...
  ASAN_MEMMOVE_IMPL(ctx, to, from, size);

Should do it. Will be folded away if PLATFORM_HAS_DIFFERENT_MEMCPY_AND_MEMMOVE is just the constant true or false.

Additionally, you can make PlatformHasDifferentMemcpyAndMemmove() cache the result of the test. If we cache it, having that extra test all the time on OS X is probably not a big deal.

[etienneb]

I'm adding more information on the bug I'm facing. (see comments above)

filcab@ and rnk@, can you comment on my case?

lib/asan/asan_interceptors.cc

416

If the runtime called asan_memcpy, it is because we actually had a call to memcpy, [...]
Oh, right. Let's not change asan_memcpy, then.

The parameters overlapping issue I'm facing when instrumenting chromium source code is a little bit subtle.
The DLL libGLESv2.dll is causing a false-positive.

This call to libglesv2!memcpy is causing the problem.

000007fe`e45fed8e 488d5101        lea     rdx,[rcx+1]
000007fe`e45fed92 e829fcfdff      call    libglesv2!memcpy (000007fe`e45de9c0)  <<------
000007fe`e45fed97 33c0            xor     eax,eax
000007fe`e45fed99 4883c420        add     rsp,20h

Within the DLL, there is an static implementation of memcpy which is hooked by ASAN. Also, when compiled, it's not rewritten by the middle-end transform (replaced by a call to __asan_memcpy).

> u libglesv2!memcpy
libglesv2!memcpy [f:\dd\vctools\crt\vcruntime\src\string\amd64\memcpy.asm @ 101]:
000007fe`e45de9c0 ff25d2167600    jmp     qword ptr [000007fe`e4d40098]                      <<-- Hooking
000007fe`e45de9c6 4983f810        cmp     r8,10h
000007fe`e45de9ca 0f8670000000    jbe     libglesv2!memcpy+0x80 (000007fe`e45dea40)
000007fe`e45de9d0 4983f820        cmp     r8,20h

The implementation is coming from the CRT.

Module name: f:\binaries\Intermediate\vctools\libvcruntime.nativeproj__1388174700\objr\amd64\memcpy.obj
Object name: D:\src\depot_tools\win_toolchain\vs_files\d5dc33b15d1b2c086f2f6632e2fd15882f80dbd3\VC\lib\amd64\libvcruntime.lib
  Symbol records:
    Symbol Type: 0x1101 S_OBJNAME (offset 0x00000004)
      Signature: 0x00000000
      Name     : f:\binaries\Intermediate\vctools\libvcruntime.nativeproj__1388174700\objr\amd64\memcpy.obj

Both memcpy and memmove in this DLL are the same.

CRT functions are hooked and redirected to the appropriate functions in the main executable.
That means both libglesv2!memcpy and libglesv2!memcpy are redirected to the main_executable!__asan_memcpy.

The dynamic hooking and thunk redirects are:

INTERCEPT_LIBRARY_FUNCTION(frexp);
INTERCEPT_LIBRARY_FUNCTION(longjmp);
#if SANITIZER_INTERCEPT_MEMCHR
INTERCEPT_LIBRARY_FUNCTION(memchr);
#endif
INTERCEPT_LIBRARY_FUNCTION(memcmp);
INTERCEPT_LIBRARY_FUNCTION(memcpy);
INTERCEPT_LIBRARY_FUNCTION(memmove);
INTERCEPT_LIBRARY_FUNCTION(memset);

I'm still not sure how the static version got statically linked into the DLL.
Here is the response file used by the linker:

D:\src\chromium\src>more out\asan64dynamic\libGLESv2.dll.rsp
advapi32.lib comdlg32.lib dbghelp.lib delayimp.lib dnsapi.lib gdi32.lib kernel32.lib msimg32.lib odbc32.lib odbccp32.lib
 ole32.lib oleaut32.lib psapi.lib shell32.lib shlwapi.lib user32.lib usp10.lib uuid.lib version.lib wininet.lib winmm.lib
 winspool.lib ws2_32.lib clang_rt.asan_dll_thunk-x86_64.lib d3d9.lib dxguid.lib

obj/third_party/angle/libGLESv2/entry_points_egl.obj
obj/third_party/angle/libGLESv2/entry_points_egl_ext.obj
obj/third_party/angle/libGLESv2/entry_points_gles_2_0.obj
obj/third_party/angle/libGLESv2/entry_points_gles_2_0_ext.obj
obj/third_party/angle/libGLESv2/entry_points_gles_3_0.obj
obj/third_party/angle/libGLESv2/entry_points_gles_3_1.obj
obj/third_party/angle/libGLESv2/global_state.obj
obj/third_party/angle/libGLESv2/libGLESv2.obj
obj/third_party/angle/libGLESv2/libGLESv2.res
obj/third_party/angle/libANGLE.lib
obj/third_party/angle/angle_common.lib
obj/third_party/angle/angle_image_util.lib
obj/third_party/angle/translator.lib
obj/third_party/angle/preprocessor.lib

/DEF:../../third_party/angle/src/libGLESv2/libGLESv2.def /OPT:REF /OPT:ICF /INCREMENTAL:NO
/FIXED:NO /OPT:ICF /DEBUG /MACHINE:X64 /FIXED:NO /ignore:4199 /ignore:4221 /NXCOMPAT
/fastfail /DYNAMICBASE /INCREMENTAL:NO /SUBSYSTEM:CONSOLE,5.02
/LIBPATH:d:/src/llvm/ninja64/lib/clang/4.0.0/lib/windows
/LIBPATH:D:/src/depot_tools/win_toolchain/vs_files/d5dc33b15d1b2c086f2f6632e2fd15882f80dbd3/win_sdk/Lib/winv6.3/um/x64
/LIBPATH:D:/src/depot_tools/win_toolchain/vs_files/d5dc33b15d1b2c086f2f6632e2fd15882f80dbd3/VC/lib/amd64

/LIBPATH:D:/src/depot_tools/win_toolchain/vs_files/d5dc33b15d1b2c086f2f6632e2fd15882f80dbd3/VC/atlmfc/lib/amd64
/DEF:../../third_party/angle/src/libGLESv2/libGLESv2.def

[etienneb]

split and simplify patch

[etienneb]

This simplified patch is fixing the issue on chromium on win7.
IIRC, there is an other issue on win10. I'll make a separate patch for it.

[etienneb]

It's working because libglesv2!memcpy and libglesv2!memcpy are redirected to

INTERCEPTOR(void*, memcpy, void *to, const void *from, uptr size) {
  void *ctx;
  ASAN_INTERCEPTOR_ENTER(ctx, memcpy);
  if (PLATFORM_HAS_DIFFERENT_MEMCPY_AND_MEMMOVE) {
    [...]
  } else {
    [...]
  }
}

[rnk]

Right, but solving your issue doesn't require changing asan_memcpy, just asan_wrap_memcpy. At least, that's what I see in the stack trace. So, the latest patch looks good to me.

The change in behavior should be exactly what we want: For pre-compiled object code we treat memcpy as memmove, but for source code compiled with ASan, we detect memcpy overlap, which would be a bug on other platforms.

[etienneb]

fix coding style (line too long)

lib/asan/asan_interceptors.cc

if (flags()->replace_intrin) {                                             \
       ASAN_READ_RANGE(ctx, from, size);                                        \
       ASAN_WRITE_RANGE(ctx, to, size);                                         \
     }                                                                          \
     return REAL(memcpy)(to, from, size);                                       \
   } while (0)
 
 
 void *__asan_memcpy(void *to, const void *from, uptr size) {
   ASAN_MEMCPY_IMPL(nullptr, to, from, size);
 }
 
 // memset is called inside Printf.
 #define ASAN_MEMSET_IMPL(ctx, block, c, size) do {                             \
     if (UNLIKELY(!asan_inited)) return internal_memset(block, c, size);        \
     if (asan_init_is_running) {                                                \
       return REAL(memset)(block, c, size);                                     \
     }                                                                          \
     ENSURE_ASAN_INITED();                                                      \
     if (flags()->replace_intrin) {                                             \
       ASAN_WRITE_RANGE(ctx, block, size);                                      \

 INTERCEPTOR(void*, memmove, void *to, const void *from, uptr size) {
   void *ctx;
   ASAN_INTERCEPTOR_ENTER(ctx, memmove);
   ASAN_MEMMOVE_IMPL(ctx, to, from, size);
 }
 
 INTERCEPTOR(void*, memcpy, void *to, const void *from, uptr size) {
   void *ctx;
   ASAN_INTERCEPTOR_ENTER(ctx, memcpy);
-#if !SANITIZER_MAC
+  if (PLATFORM_HAS_DIFFERENT_MEMCPY_AND_MEMMOVE) {
     ASAN_MEMCPY_IMPL(ctx, to, from, size);
-#else
+  } else {
     // At least on 10.7 and 10.8 both memcpy() and memmove() are being replaced
-  // with WRAP(memcpy). As a result, false positives are reported for memmove()
-  // calls. If we just disable error reporting with
+    // with WRAP(memcpy). As a result, false positives are reported for
+    // memmove() calls. If we just disable error reporting with
     // ASAN_OPTIONS=replace_intrin=0, memmove() is still replaced with
     // internal_memcpy(), which may lead to crashes, see
     // http://llvm.org/bugs/show_bug.cgi?id=16362.
     ASAN_MEMMOVE_IMPL(ctx, to, from, size);
-#endif  // !SANITIZER_MAC
+  }
 }
 
 INTERCEPTOR(void*, memset, void *block, int c, uptr size) {
   void *ctx;
   ASAN_INTERCEPTOR_ENTER(ctx, memset);
   ASAN_MEMSET_IMPL(ctx, block, c, size);
 }