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

Wed Dec 7 07:57:34 PST 2016

etienneb added a comment.

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

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

================
Comment at: lib/asan/asan_interceptors.cc:427
+  ASAN_MEMMOVE_IMPL(nullptr, to, from, size);
+#endif
+}
----------------
rnk wrote:
> filcab wrote:
> > rnk wrote:
> > > filcab wrote:
> > > > 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.
> > > 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.
> > 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)
> Oh, right. Let's not change __asan_memcpy, then.
> 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
```

https://reviews.llvm.org/D27052