Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2984933002:
Allocator shims design prototype. (Closed)
DescriptionThis is a prototype of the allocator shim.
Currently it compiles and the integration tests compile and link and use the allocator shims. It's been tested that the methods from the shims are used not the ones ucrt.
Other comments and questions on the design are welcome.
BUG=
Review-Url: https://codereview.chromium.org/2984933002
Committed: https://github.com/google/syzygy/commit/d0372eb9d3bbf7451a908762babbe8e9c08cccef
Patch Set 1 #Patch Set 2 : Removing files nto needed for this branch #
Total comments: 37
Patch Set 3 : Response to reviewers comments. #
Total comments: 6
Patch Set 4 : Response to reviewers comments. #Patch Set 5 : Removed the set new mode methods.c #
Total comments: 8
Patch Set 6 : Response to reviewers comments. #Patch Set 7 : Response to reviewers comments and added missing asan_heap initialization. #
Total comments: 20
Patch Set 8 : Response to reviewers comments. #
Total comments: 6
Patch Set 9 : Response to reviewer comments. #
Total comments: 14
Patch Set 10 : Response to reviers comments. #
Total comments: 6
Patch Set 11 : Response to reviewers comments. #
Dependent Patchsets: Messages
Total messages: 61 (30 generated)
Description was changed from ========== Allocator shims design prototype. BUG= ========== to ========== Allocator shims design prototype. BUG= ==========
njanevsk@google.com changed reviewers: + chrisha@chromium.org, sebmarchand@chromium.org
This is a prototype of the allocator shim. Currently when compiling it give a link error that the following two methods are not found: integration_tests_clang_dll.allocator_shim.obj : error LNK2019: unresolved external symbol ___asan_before_dynamic_init referenced in function __GLOBAL__sub_I_allocator_shim.cc integration_tests_clang_dll.allocator_shim.obj : error LNK2019: unresolved external symbol ___asan_after_dynamic_init referenced in function __GLOBAL__sub_I_allocator_shim.cc Should I create a stub methods for them? They are used because HMODUL asan_module is a global variable as part of the structure. If it is not a global variable then this methods are not needed. Other comments and questions on the design are welcome.
This is a prototype of the allocator shim. Currently when compiling it give a link error that the following two methods are not found: integration_tests_clang_dll.allocator_shim.obj : error LNK2019: unresolved external symbol ___asan_before_dynamic_init referenced in function __GLOBAL__sub_I_allocator_shim.cc integration_tests_clang_dll.allocator_shim.obj : error LNK2019: unresolved external symbol ___asan_after_dynamic_init referenced in function __GLOBAL__sub_I_allocator_shim.cc Should I create a stub methods for them? They are used because HMODUL asan_module is a global variable as part of the structure. If it is not a global variable then this methods are not needed. Other comments and questions on the design are welcome.
Could you remove all the changes that are not about the allocator shim? (i.e. the parameterized test things etc), this CL should only include the change to the gyp file and the new allocator_shim file (i.e. no change in the unittests etc). Also please move the description of the change in the CL's description rather than in your first comment.
The CQ bit was checked by njanevsk@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Allocator shims design prototype. BUG= ========== to ========== This is a prototype of the allocator shim. Currently when compiling it give a link error that the following two methods are not found: integration_tests_clang_dll.allocator_shim.obj : error LNK2019: unresolved external symbol ___asan_before_dynamic_init referenced in function __GLOBAL__sub_I_allocator_shim.cc integration_tests_clang_dll.allocator_shim.obj : error LNK2019: unresolved external symbol ___asan_after_dynamic_init referenced in function __GLOBAL__sub_I_allocator_shim.cc Should I create a stub methods for them? They are used because HMODUL asan_module is a global variable as part of the structure. If it is not a global variable then this methods are not needed. Other comments and questions on the design are welcome. BUG= ==========
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_dbg_try on master.tryserver.client.syzygy (JOB_FAILED, https://build.chromium.org/p/tryserver.client.syzygy/builders/win_dbg_try/bui...)
https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... File syzygy/integration_tests/allocator_shim.cc (right): https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:5: // This header defines symbols to override the same functions in the Visual C++ This isn't a header :) https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:9: #error This header is meant to be included only once by allocator_shim.cc We don't need this anymore. https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:16: #include "base/logging.h" Do you really need logging? https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:22: typedef HANDLE(WINAPI* HeapCreatePtr)(DWORD, SIZE_T, SIZE_T); Add a comment to describe what these typedefs are. https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:28: int win_new_mode = 0; You don't need this. https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:29: std::mutex m; Rename this to something more meaningful, add a comment to describe what it's for. Move the declaration next to the asan_shim one. https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:30: Remove one of these BLs https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:32: struct asan_shim_struct { Use a CamelCase name. https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:44: std::unique_lock<std::mutex> lock(m); I think that we could avoid all this by moving this code to the constructor of the asan_shim struct, Chris, wdyt? https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:60: load_asan_module(); put this behind a "if (asan_shim.asan_module == nullptr)", otherwise you'll grab the lock for nothing. https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:84: Remove this BL. https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:89: load_asan_module(); This is confusing, the load_asan_module is in some functions but not in the others, you could add a "GetAllocShim" function that returns a pointer to the initialized struct instead. https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:101: ::memset(ptr, 0, size * n); Check if ptr == nullptr before this, the malloc call might fail in which case this function should return nullptr (http://www.cplusplus.com/reference/cstdlib/calloc/) https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:129: // The default dispatch translation unit has to define also the following You don't need this comment afaik.
Chris we need your feedback here. PTAL. https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... File syzygy/integration_tests/allocator_shim.cc (right): https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/07/25 16:21:53, Sébastien Marchand wrote: > 2017 Done. https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:5: // This header defines symbols to override the same functions in the Visual C++ On 2017/07/25 16:21:53, Sébastien Marchand wrote: > This isn't a header :) Done. https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:9: #error This header is meant to be included only once by allocator_shim.cc On 2017/07/25 16:21:53, Sébastien Marchand wrote: > We don't need this anymore. Done. https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:22: typedef HANDLE(WINAPI* HeapCreatePtr)(DWORD, SIZE_T, SIZE_T); On 2017/07/25 16:21:53, Sébastien Marchand wrote: > Add a comment to describe what these typedefs are. Done. https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:28: int win_new_mode = 0; On 2017/07/25 16:21:53, Sébastien Marchand wrote: > You don't need this. Done. https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:29: std::mutex m; On 2017/07/25 16:21:53, Sébastien Marchand wrote: > Rename this to something more meaningful, add a comment to describe what it's > for. Move the declaration next to the asan_shim one. Done. https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:30: On 2017/07/25 16:21:52, Sébastien Marchand wrote: > Remove one of these BLs Done. https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:32: struct asan_shim_struct { On 2017/07/25 16:21:53, Sébastien Marchand wrote: > Use a CamelCase name. Done. https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:44: std::unique_lock<std::mutex> lock(m); On 2017/07/25 16:21:52, Sébastien Marchand wrote: > I think that we could avoid all this by moving this code to the constructor of > the asan_shim struct, Chris, wdyt? That sounds like a really good idea. That way the variable will be initialized when created and we can get rid of the load_asan_module calls in each function. https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:84: On 2017/07/25 16:21:53, Sébastien Marchand wrote: > Remove this BL. Done. https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:89: load_asan_module(); On 2017/07/25 16:21:52, Sébastien Marchand wrote: > This is confusing, the load_asan_module is in some functions but not in the > others, you could add a "GetAllocShim" function that returns a pointer to the > initialized struct instead. If we use a constructor for the structure then the asan_shim structure will always be initialized and there will not be a need to call it in any of the methods. https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:101: ::memset(ptr, 0, size * n); On 2017/07/25 16:21:53, Sébastien Marchand wrote: > Check if ptr == nullptr before this, the malloc call might fail in which case > this function should return nullptr > (http://www.cplusplus.com/reference/cstdlib/calloc/) Done. https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:129: // The default dispatch translation unit has to define also the following On 2017/07/25 16:21:53, Sébastien Marchand wrote: > You don't need this comment afaik. Done.
https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... File syzygy/integration_tests/allocator_shim.cc (right): https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:32: struct asan_shim_struct { On 2017/07/25 17:43:02, njanevsk wrote: > On 2017/07/25 16:21:53, Sébastien Marchand wrote: > > Use a CamelCase name. > > Done. Also, _struct / Struct isn't adding any additional information. This function is a collection of points to runtime library heap functions, so maybe a better name would be RuntimePointers? https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:38: HeapReAllocPtr heap_realloc; All of these should have = nullptr as well. https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:44: std::unique_lock<std::mutex> lock(m); On 2017/07/25 17:43:02, njanevsk wrote: > On 2017/07/25 16:21:52, Sébastien Marchand wrote: > > I think that we could avoid all this by moving this code to the constructor of > > the asan_shim struct, Chris, wdyt? > > That sounds like a really good idea. That way the variable will be initialized > when created and we can get rid of the load_asan_module calls in each function. That means that we rely on the static initializers of this module having run before any heap allocations can be made. But, other static initializers running before ours might need to make heap allocations. I'd stick with the current mechanism (a gateway function for the initialization). I'd also move the lock acquisition inside of the check for asan_module == nullptr. In fact, you could get rid of the lock entirely, as we know we are running on x86 systems where 32-bit reads and writes are atomic. The worst case scenario is that multiple threads do the resolving at the same time, each coming up with the same answer. Finally, I'd call this functions something more meaningful, like ResolveRuntimePointers, maybe?
I haven't received response to my question in the description of the CL. I'll have to add stub methods for the two asan functions to get the code to compile. PTAL. https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... File syzygy/integration_tests/allocator_shim.cc (right): https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:16: #include "base/logging.h" On 2017/07/25 16:21:53, Sébastien Marchand wrote: > Do you really need logging? Done. https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:32: struct asan_shim_struct { On 2017/07/25 18:47:14, chrisha wrote: > On 2017/07/25 17:43:02, njanevsk wrote: > > On 2017/07/25 16:21:53, Sébastien Marchand wrote: > > > Use a CamelCase name. > > > > Done. > > Also, _struct / Struct isn't adding any additional information. This function is > a collection of points to runtime library heap functions, so maybe a better name > would be RuntimePointers? I changed it to AsanRuntimePointers. I want to have Asan in the name to make it more specific and descriptive. RuntimePointers is general. https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:38: HeapReAllocPtr heap_realloc; On 2017/07/25 18:47:14, chrisha wrote: > All of these should have = nullptr as well. Done. https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:44: std::unique_lock<std::mutex> lock(m); On 2017/07/25 18:47:14, chrisha wrote: > On 2017/07/25 17:43:02, njanevsk wrote: > > On 2017/07/25 16:21:52, Sébastien Marchand wrote: > > > I think that we could avoid all this by moving this code to the constructor > of > > > the asan_shim struct, Chris, wdyt? > > > > That sounds like a really good idea. That way the variable will be initialized > > when created and we can get rid of the load_asan_module calls in each > function. > > That means that we rely on the static initializers of this module having run > before any heap allocations can be made. But, other static initializers running > before ours might need to make heap allocations. I'd stick with the current > mechanism (a gateway function for the initialization). > > I'd also move the lock acquisition inside of the check for asan_module == > nullptr. > > In fact, you could get rid of the lock entirely, as we know we are running on > x86 systems where 32-bit reads and writes are atomic. The worst case scenario is > that multiple threads do the resolving at the same time, each coming up with the > same answer. > > Finally, I'd call this functions something more meaningful, like > ResolveRuntimePointers, maybe? Thanks. I removed the lock. Seb are you satisified with this solution? I renamed the function to: resolve_asan_runtime_pointers() Again I am using asan to make it more specific and in agreement with the data structure's name. https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:60: load_asan_module(); On 2017/07/25 16:21:53, Sébastien Marchand wrote: > put this behind a "if (asan_shim.asan_module == nullptr)", otherwise you'll grab > the lock for nothing. Since we are getting rid of the lock this is N\A.
I'd prefer to not have to add the ___asan_after_dynamic_init / ___asan_before_dynamic_init functions just because we need them here, could you use the no_sanitize attribute on the variable instead? Type __attribute__((no_sanitize_address)) variable_name; https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... File syzygy/integration_tests/allocator_shim.cc (right): https://codereview.chromium.org/2984933002/diff/20001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:44: std::unique_lock<std::mutex> lock(m); On 2017/07/25 19:49:17, njanevsk wrote: > On 2017/07/25 18:47:14, chrisha wrote: > > On 2017/07/25 17:43:02, njanevsk wrote: > > > On 2017/07/25 16:21:52, Sébastien Marchand wrote: > > > > I think that we could avoid all this by moving this code to the > constructor > > of > > > > the asan_shim struct, Chris, wdyt? > > > > > > That sounds like a really good idea. That way the variable will be > initialized > > > when created and we can get rid of the load_asan_module calls in each > > function. > > > > That means that we rely on the static initializers of this module having run > > before any heap allocations can be made. But, other static initializers > running > > before ours might need to make heap allocations. I'd stick with the current > > mechanism (a gateway function for the initialization). > > > > I'd also move the lock acquisition inside of the check for asan_module == > > nullptr. > > > > In fact, you could get rid of the lock entirely, as we know we are running on > > x86 systems where 32-bit reads and writes are atomic. The worst case scenario > is > > that multiple threads do the resolving at the same time, each coming up with > the > > same answer. > > > > Finally, I'd call this functions something more meaningful, like > > ResolveRuntimePointers, maybe? > Thanks. I removed the lock. Seb are you satisified with this solution? > I renamed the function to: > resolve_asan_runtime_pointers() > Again I am using asan to make it more specific and in agreement with the data > structure's name. Yep, this approach sgtm. https://codereview.chromium.org/2984933002/diff/40001/syzygy/integration_test... File syzygy/integration_tests/allocator_shim.cc (right): https://codereview.chromium.org/2984933002/diff/40001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:17: // from the ucrt library that we are overwritting with syzyasan_rtl.dll. That's not true, these functions are heap pointers for the heap functions provided by SyzyAsan, we're not overwriting any of the heap_* functions here, we're just using them in the memory management functions (malloc/free/...). So maybe you could just say: "Definition of the heap management functions exposed by the SyzyAsan runtime library"? https://codereview.chromium.org/2984933002/diff/40001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:35: void resolve_asan_runtime_pointers() { CamelCase. https://codereview.chromium.org/2984933002/diff/40001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:74: resolve_asan_runtime_pointers(); I don't really like having these calls in every functions, what about adding something like: const AsanRuntimePointers* GetAsanHeapShim() { static AsanRuntimePointers asan_runtime_ptrs; if (asan_shim.asan_module == nullptr) { resolve_asan_runtime_pointers(&asan_runtimr_ptrs); } return &asan_runtime_ptrs; } and in these functions you'll just do: GetAsanHeapShim()->heap_free(...);
njanevsk@google.com changed reviewers: + siggi@chromium.org
PTAL https://codereview.chromium.org/2984933002/diff/40001/syzygy/integration_test... File syzygy/integration_tests/allocator_shim.cc (right): https://codereview.chromium.org/2984933002/diff/40001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:17: // from the ucrt library that we are overwritting with syzyasan_rtl.dll. On 2017/07/25 20:07:07, Sébastien Marchand wrote: > That's not true, these functions are heap pointers for the heap functions > provided by SyzyAsan, we're not overwriting any of the heap_* functions here, > we're just using them in the memory management functions (malloc/free/...). > > So maybe you could just say: "Definition of the heap management functions > exposed by the SyzyAsan runtime library"? Done. https://codereview.chromium.org/2984933002/diff/40001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:35: void resolve_asan_runtime_pointers() { On 2017/07/25 20:07:07, Sébastien Marchand wrote: > CamelCase. Done. https://codereview.chromium.org/2984933002/diff/40001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:74: resolve_asan_runtime_pointers(); On 2017/07/25 20:07:07, Sébastien Marchand wrote: > I don't really like having these calls in every functions, what about adding > something like: > const AsanRuntimePointers* GetAsanHeapShim() { > static AsanRuntimePointers asan_runtime_ptrs; > if (asan_shim.asan_module == nullptr) { > resolve_asan_runtime_pointers(&asan_runtimr_ptrs); > } > return &asan_runtime_ptrs; > } > > and in these functions you'll just do: > GetAsanHeapShim()->heap_free(...); > Done.
Removed a line of commented out dead code.
Basic approach looks good, but it's be good to get another pair of eyes on this. https://codereview.chromium.org/2984933002/diff/80001/syzygy/integration_test... File syzygy/integration_tests/allocator_shim.cc (right): https://codereview.chromium.org/2984933002/diff/80001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:35: void ResolveAsanRuntimePointers(AsanRuntimePointers* asan_runtime_ptrs) { Add a comment to describe what this function does. https://codereview.chromium.org/2984933002/diff/80001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:49: const AsanRuntimePointers* GetAsanHeapShim() { Ditto, comment. https://codereview.chromium.org/2984933002/diff/80001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:50: static AsanRuntimePointers asan_runtime_ptrs; Add "= {}" https://codereview.chromium.org/2984933002/diff/80001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:93: __acrt_heap = ::HeapCreate(0, 0, 0); Ideally I'd like to use an Asan heap here too but I don't think that it's possible (was crashing when I tried it locally, I'll need to investigate). Can you add a TODO (assigned to me) to check if it'll be possible to use an Asan heap?
Fixed the comments and discovered that I was not initializing the asan_heap variable. Now, I fixed it and it might be possible to use asan_heap in _acrt_initialize_heap. https://codereview.chromium.org/2984933002/diff/80001/syzygy/integration_test... File syzygy/integration_tests/allocator_shim.cc (right): https://codereview.chromium.org/2984933002/diff/80001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:35: void ResolveAsanRuntimePointers(AsanRuntimePointers* asan_runtime_ptrs) { On 2017/07/26 19:12:02, Sébastien Marchand wrote: > Add a comment to describe what this function does. Done. https://codereview.chromium.org/2984933002/diff/80001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:49: const AsanRuntimePointers* GetAsanHeapShim() { On 2017/07/26 19:12:02, Sébastien Marchand wrote: > Ditto, comment. Done. https://codereview.chromium.org/2984933002/diff/80001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:50: static AsanRuntimePointers asan_runtime_ptrs; On 2017/07/26 19:12:02, Sébastien Marchand wrote: > Add "= {}" Done. https://codereview.chromium.org/2984933002/diff/80001/syzygy/integration_test... syzygy/integration_tests/allocator_shim.cc:93: __acrt_heap = ::HeapCreate(0, 0, 0); On 2017/07/26 19:12:02, Sébastien Marchand wrote: > Ideally I'd like to use an Asan heap here too but I don't think that it's > possible (was crashing when I tried it locally, I'll need to investigate). > > Can you add a TODO (assigned to me) to check if it'll be possible to use an Asan > heap? Done.
Description was changed from ========== This is a prototype of the allocator shim. Currently when compiling it give a link error that the following two methods are not found: integration_tests_clang_dll.allocator_shim.obj : error LNK2019: unresolved external symbol ___asan_before_dynamic_init referenced in function __GLOBAL__sub_I_allocator_shim.cc integration_tests_clang_dll.allocator_shim.obj : error LNK2019: unresolved external symbol ___asan_after_dynamic_init referenced in function __GLOBAL__sub_I_allocator_shim.cc Should I create a stub methods for them? They are used because HMODUL asan_module is a global variable as part of the structure. If it is not a global variable then this methods are not needed. Other comments and questions on the design are welcome. BUG= ========== to ========== This is a prototype of the allocator shim. Other comments and questions on the design are welcome. BUG= ==========
Description was changed from ========== This is a prototype of the allocator shim. Other comments and questions on the design are welcome. BUG= ========== to ========== This is a prototype of the allocator shim. Currently it compiles and the integration tests compile and link and use the allocator shims. It's been tested that the methods from the shims are used not the ones ucrt. Other comments and questions on the design are welcome. BUG= ==========
https://codereview.chromium.org/2984933002/diff/120001/syzygy/integration_tes... File syzygy/integration_tests/allocator_shim.cc (right): https://codereview.chromium.org/2984933002/diff/120001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:37: asan_runtime_ptrs->asan_module = GetModuleHandle(L"syzyasan_rtl.dll"); if this returns nullptr, then the calls below may malfunction (GetProcAddress(nullptr, ...) has defined behavior. Note also that GetModuleHandle does not load the module, it merely retrieves it if it's already loaded into the process. https://codereview.chromium.org/2984933002/diff/120001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:54: static AsanRuntimePointers asan_runtime_ptrs = {}; In Chrome land, we've now started to rely on thread-safe static initialization. Maybe that's safer and faster here? https://codereview.chromium.org/2984933002/diff/120001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:69: return GetAsanHeapShim()->heap_alloc(get_heap_handle(), 0, size); does compiler do a decent job of eliminating the duplicate call to GetAsanHeapShim? Traversing through a function that contains a statically initialized variable might be expensive (depending on how the compiler handles it), so could be worth explicitly retrieving the shim and using it twice... https://codereview.chromium.org/2984933002/diff/120001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:96: bool __acrt_initialize_heap() { if you intend this for use in Chrome, then it won't work as-is. There are cases where Chrome calls _get_heap_handle, IIRC, and you're going to return the wrong one, methinks?
https://codereview.chromium.org/2984933002/diff/120001/syzygy/integration_tes... File syzygy/integration_tests/allocator_shim.cc (right): https://codereview.chromium.org/2984933002/diff/120001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:33: // If the syzyasan_rtl.dll is not loaded it loads it and the heap functions This comment isn't true, it only retrieves the module handle but doesn't load it https://codereview.chromium.org/2984933002/diff/120001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:37: asan_runtime_ptrs->asan_module = GetModuleHandle(L"syzyasan_rtl.dll"); On 2017/07/28 17:03:14, Sigurður Ásgeirsson wrote: > if this returns nullptr, then the calls below may malfunction > (GetProcAddress(nullptr, ...) has defined behavior. > > Note also that GetModuleHandle does not load the module, it merely retrieves it > if it's already loaded into the process. Ha good point, it's probably safe as-is for the integration tests but adding some safety checks will help to prevent some flakiness https://codereview.chromium.org/2984933002/diff/120001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:52: // the heap methods. Move this to the previous line. https://codereview.chromium.org/2984933002/diff/120001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:54: static AsanRuntimePointers asan_runtime_ptrs = {}; On 2017/07/28 17:03:14, Sigurður Ásgeirsson wrote: > In Chrome land, we've now started to rely on thread-safe static initialization. > Maybe that's safer and faster here? Ha, that's cool. In this case you could add a constructor to AsanRuntimePtr and do: static AsanRuntimePointers* asan_runtime_ptrs = new AsanRuntimePointers(); https://codereview.chromium.org/2984933002/diff/120001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:69: return GetAsanHeapShim()->heap_alloc(get_heap_handle(), 0, size); On 2017/07/28 17:03:14, Sigurður Ásgeirsson wrote: > does compiler do a decent job of eliminating the duplicate call to > GetAsanHeapShim? Traversing through a function that contains a statically > initialized variable might be expensive (depending on how the compiler handles > it), so could be worth explicitly retrieving the shim and using it twice... Not that this shim is only intended to be used in the context of the integration tests so performance isn't a huge criteria but it'd be interesting to look at the generated code to see how Clang handle this. https://codereview.chromium.org/2984933002/diff/120001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:96: bool __acrt_initialize_heap() { On 2017/07/28 17:03:14, Sigurður Ásgeirsson wrote: > if you intend this for use in Chrome, then it won't work as-is. There are cases > where Chrome calls _get_heap_handle, IIRC, and you're going to return the wrong > one, methinks? This shim is not meant to be used in Chrome.
The CQ bit was checked by njanevsk@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2984933002/diff/120001/syzygy/integration_tes... File syzygy/integration_tests/allocator_shim.cc (right): https://codereview.chromium.org/2984933002/diff/120001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:33: // If the syzyasan_rtl.dll is not loaded it loads it and the heap functions On 2017/07/31 15:01:40, Sébastien Marchand wrote: > This comment isn't true, it only retrieves the module handle but doesn't load it Done. https://codereview.chromium.org/2984933002/diff/120001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:37: asan_runtime_ptrs->asan_module = GetModuleHandle(L"syzyasan_rtl.dll"); On 2017/07/31 15:01:40, Sébastien Marchand wrote: > On 2017/07/28 17:03:14, Sigurður Ásgeirsson wrote: > > if this returns nullptr, then the calls below may malfunction > > (GetProcAddress(nullptr, ...) has defined behavior. > > > > Note also that GetModuleHandle does not load the module, it merely retrieves > it > > if it's already loaded into the process. > > Ha good point, it's probably safe as-is for the integration tests but adding > some safety checks will help to prevent some flakiness I added a check to make sure that the module is retrieved. However, this does not exit the code. How do you handle this in here? Do you throw an exception or modify all the methods to check that GetAsanHeapShim does not return nullptr? https://codereview.chromium.org/2984933002/diff/120001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:52: // the heap methods. On 2017/07/31 15:01:40, Sébastien Marchand wrote: > Move this to the previous line. Done. https://codereview.chromium.org/2984933002/diff/120001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:54: static AsanRuntimePointers asan_runtime_ptrs = {}; On 2017/07/31 15:01:40, Sébastien Marchand wrote: > On 2017/07/28 17:03:14, Sigurður Ásgeirsson wrote: > > In Chrome land, we've now started to rely on thread-safe static > initialization. > > Maybe that's safer and faster here? > > Ha, that's cool. In this case you could add a constructor to AsanRuntimePtr and > do: > static AsanRuntimePointers* asan_runtime_ptrs = new AsanRuntimePointers(); Done. https://codereview.chromium.org/2984933002/diff/120001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:69: return GetAsanHeapShim()->heap_alloc(get_heap_handle(), 0, size); On 2017/07/31 15:01:40, Sébastien Marchand wrote: > On 2017/07/28 17:03:14, Sigurður Ásgeirsson wrote: > > does compiler do a decent job of eliminating the duplicate call to > > GetAsanHeapShim? Traversing through a function that contains a statically > > initialized variable might be expensive (depending on how the compiler handles > > it), so could be worth explicitly retrieving the shim and using it twice... > > Not that this shim is only intended to be used in the context of the integration > tests so performance isn't a huge criteria but it'd be interesting to look at > the generated code to see how Clang handle this. Leaving it as it is for now. https://codereview.chromium.org/2984933002/diff/120001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:96: bool __acrt_initialize_heap() { On 2017/07/31 15:01:40, Sébastien Marchand wrote: > On 2017/07/28 17:03:14, Sigurður Ásgeirsson wrote: > > if you intend this for use in Chrome, then it won't work as-is. There are > cases > > where Chrome calls _get_heap_handle, IIRC, and you're going to return the > wrong > > one, methinks? > > This shim is not meant to be used in Chrome. Not fixing cause it is not meant to be use with Chrome.
https://codereview.chromium.org/2984933002/diff/120001/syzygy/integration_tes... File syzygy/integration_tests/allocator_shim.cc (right): https://codereview.chromium.org/2984933002/diff/120001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:96: bool __acrt_initialize_heap() { On 2017/07/31 18:57:28, njanevsk wrote: > On 2017/07/31 15:01:40, Sébastien Marchand wrote: > > On 2017/07/28 17:03:14, Sigurður Ásgeirsson wrote: > > > if you intend this for use in Chrome, then it won't work as-is. There are > > cases > > > where Chrome calls _get_heap_handle, IIRC, and you're going to return the > > wrong > > > one, methinks? > > > > This shim is not meant to be used in Chrome. > > Not fixing cause it is not meant to be use with Chrome. I don't understand why you'd want the integration test to implement incorrect behavior, though. Why not use the same heap here and in the shim functions? https://codereview.chromium.org/2984933002/diff/140001/syzygy/integration_tes... File syzygy/integration_tests/allocator_shim.cc (right): https://codereview.chromium.org/2984933002/diff/140001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:35: // It retreives the handle for the syzyasan_rtl.dll module nit: speling [sic] https://codereview.chromium.org/2984933002/diff/140001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:59: static AsanRuntimePointers* asan_runtime_ptrs = new AsanRuntimePointers(); better for this to be a straight-up member than a pointer. As-is, the allocation (may) leak(s) on unload. https://codereview.chromium.org/2984933002/diff/140001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:60: if (asan_runtime_ptrs->asan_module == nullptr) { This is not threadsafe, plus there's the question of whether you want negative caching here. I would just shunt this work into the constructor, as there really isn't any fallback in case of error anyhow.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by njanevsk@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL. https://codereview.chromium.org/2984933002/diff/120001/syzygy/integration_tes... File syzygy/integration_tests/allocator_shim.cc (right): https://codereview.chromium.org/2984933002/diff/120001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:96: bool __acrt_initialize_heap() { On 2017/07/31 19:05:52, Sigurður Ásgeirsson wrote: > On 2017/07/31 18:57:28, njanevsk wrote: > > On 2017/07/31 15:01:40, Sébastien Marchand wrote: > > > On 2017/07/28 17:03:14, Sigurður Ásgeirsson wrote: > > > > if you intend this for use in Chrome, then it won't work as-is. There are > > > cases > > > > where Chrome calls _get_heap_handle, IIRC, and you're going to return the > > > wrong > > > > one, methinks? > > > > > > This shim is not meant to be used in Chrome. > > > > Not fixing cause it is not meant to be use with Chrome. > > I don't understand why you'd want the integration test to implement incorrect > behavior, though. Why not use the same heap here and in the shim functions? I tried using the same heap but the Clang test cases are crushing in that case. Seb might be able to provide a better answer. https://codereview.chromium.org/2984933002/diff/140001/syzygy/integration_tes... File syzygy/integration_tests/allocator_shim.cc (right): https://codereview.chromium.org/2984933002/diff/140001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:35: // It retreives the handle for the syzyasan_rtl.dll module On 2017/07/31 19:05:52, Sigurður Ásgeirsson wrote: > nit: speling [sic] Done. https://codereview.chromium.org/2984933002/diff/140001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:59: static AsanRuntimePointers* asan_runtime_ptrs = new AsanRuntimePointers(); On 2017/07/31 19:05:52, Sigurður Ásgeirsson wrote: > better for this to be a straight-up member than a pointer. As-is, the allocation > (may) leak(s) on unload. Done. https://codereview.chromium.org/2984933002/diff/140001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:60: if (asan_runtime_ptrs->asan_module == nullptr) { On 2017/07/31 19:05:52, Sigurður Ásgeirsson wrote: > This is not threadsafe, plus there's the question of whether you want negative > caching here. I would just shunt this work into the constructor, as there really > isn't any fallback in case of error anyhow. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2984933002/diff/120001/syzygy/integration_tes... File syzygy/integration_tests/allocator_shim.cc (right): https://codereview.chromium.org/2984933002/diff/120001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:96: bool __acrt_initialize_heap() { On 2017/08/01 15:46:43, njanevsk wrote: > On 2017/07/31 19:05:52, Sigurður Ásgeirsson wrote: > > On 2017/07/31 18:57:28, njanevsk wrote: > > > On 2017/07/31 15:01:40, Sébastien Marchand wrote: > > > > On 2017/07/28 17:03:14, Sigurður Ásgeirsson wrote: > > > > > if you intend this for use in Chrome, then it won't work as-is. There > are > > > > cases > > > > > where Chrome calls _get_heap_handle, IIRC, and you're going to return > the > > > > wrong > > > > > one, methinks? > > > > > > > > This shim is not meant to be used in Chrome. > > > > > > Not fixing cause it is not meant to be use with Chrome. > > > > I don't understand why you'd want the integration test to implement incorrect > > behavior, though. Why not use the same heap here and in the shim functions? > > I tried using the same heap but the Clang test cases are crushing in that case. > Seb might be able to provide a better answer. The problem here is that we can't use a heap that hasn't been created by ::HeapCreate to initialize __acrt_heap. The DLL main code ends up calling _calloc_base directly, which calls ::HeapAlloc(__acrt_heap,...) and we're not patching HeapAlloc here. Note that this code comes from Chrome: https://cs.chromium.org/chromium/src/base/allocator/allocator_shim_override_u... This means that we won't have coverage for all the initialization made directly by the CRT via HeapAlloc, this is probably fine because we don't instrument this code with Clang anyway.
https://codereview.chromium.org/2984933002/diff/120001/syzygy/integration_tes... File syzygy/integration_tests/allocator_shim.cc (right): https://codereview.chromium.org/2984933002/diff/120001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:96: bool __acrt_initialize_heap() { On 2017/08/03 21:49:45, Sébastien Marchand wrote: > On 2017/08/01 15:46:43, njanevsk wrote: > > On 2017/07/31 19:05:52, Sigurður Ásgeirsson wrote: > > > On 2017/07/31 18:57:28, njanevsk wrote: > > > > On 2017/07/31 15:01:40, Sébastien Marchand wrote: > > > > > On 2017/07/28 17:03:14, Sigurður Ásgeirsson wrote: > > > > > > if you intend this for use in Chrome, then it won't work as-is. There > > are > > > > > cases > > > > > > where Chrome calls _get_heap_handle, IIRC, and you're going to return > > the > > > > > wrong > > > > > > one, methinks? > > > > > > > > > > This shim is not meant to be used in Chrome. > > > > > > > > Not fixing cause it is not meant to be use with Chrome. > > > > > > I don't understand why you'd want the integration test to implement > incorrect > > > behavior, though. Why not use the same heap here and in the shim functions? > > > > I tried using the same heap but the Clang test cases are crushing in that > case. > > Seb might be able to provide a better answer. > > The problem here is that we can't use a heap that hasn't been created by > ::HeapCreate to initialize __acrt_heap. The DLL main code ends up calling > _calloc_base directly, which calls ::HeapAlloc(__acrt_heap,...) and we're not > patching HeapAlloc here. > > Note that this code comes from Chrome: > https://cs.chromium.org/chromium/src/base/allocator/allocator_shim_override_u... > > This means that we won't have coverage for all the initialization made directly > by the CRT via HeapAlloc, this is probably fine because we don't instrument this > code with Clang anyway. > No, this isn't OK, and this WILL fail in Chrome, so your integration tests here aren't going to be any good for what you want to use this for eventually. What's going to happen when the pointer out of _calloc_base is freed?
On 2017/08/04 13:55:14, Sigurður Ásgeirsson wrote: > No, this isn't OK, and this WILL fail in Chrome, so your integration tests here > aren't going to be any good for what you want to use this for eventually. What's > going to happen when the pointer out of _calloc_base is freed? K, per our offline chat, you're of the opinion that the _*_base functions are a layer down, and strictly mated together. If so, then this'll work in Chrome, though the _*_base allocations won't be asanified.
Yep, all these functions directly talk to the heap (via ::Heap*) and don't use the CRT's memory management functions (i.e. the core of the CRT doesn't depend on itself)
https://codereview.chromium.org/2984933002/diff/160001/syzygy/integration_tes... File syzygy/integration_tests/allocator_shim.cc (right): https://codereview.chromium.org/2984933002/diff/160001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:17: // heap_create, heap_destroy, heap_alloc, heap_realloc, and heap_free Add a period. https://codereview.chromium.org/2984933002/diff/160001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:26: HANDLE asan_heap = nullptr; Move these after the constructor as specified in the styleguide: https://google.github.io/styleguide/cppguide.html#Declaration_Order https://codereview.chromium.org/2984933002/diff/160001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:34: // It retrieves the handle for the syzyasan_rtl.dll module This should go in the body of the function. https://codereview.chromium.org/2984933002/diff/160001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:56: AsanRuntimePointers __attribute__((no_sanitize_address)) AsanRuntimePointers::asan_runtime; This is > 80 characters. https://codereview.chromium.org/2984933002/diff/160001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:56: AsanRuntimePointers __attribute__((no_sanitize_address)) AsanRuntimePointers::asan_runtime; Add a comment to explain why you need to use the no_sanitize_address attribute. https://codereview.chromium.org/2984933002/diff/160001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:60: const AsanRuntimePointers* GetAsanHeapShim() { Do we need this? Couldn't we use AsanRuntimePointers::asan_runtime directly? https://codereview.chromium.org/2984933002/diff/160001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:100: // TODO(sebmarchand): Check if it is possible to use asan_heap here. Remove this TODO, add something like this instead: // The core CRT functions don't use the CRT's memory management // functions, instead they directly use |__acrt_heap| and calls the // ::Heap* functions. Because of this it's not possible to replace this // heap by an Asan one.
PTAL!
PTAL!
Marked comments as Done. https://codereview.chromium.org/2984933002/diff/160001/syzygy/integration_tes... File syzygy/integration_tests/allocator_shim.cc (right): https://codereview.chromium.org/2984933002/diff/160001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:17: // heap_create, heap_destroy, heap_alloc, heap_realloc, and heap_free On 2017/08/08 15:11:55, Sébastien Marchand wrote: > Add a period. Done. https://codereview.chromium.org/2984933002/diff/160001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:26: HANDLE asan_heap = nullptr; On 2017/08/08 15:11:55, Sébastien Marchand wrote: > Move these after the constructor as specified in the styleguide: > https://google.github.io/styleguide/cppguide.html#Declaration_Order Done. https://codereview.chromium.org/2984933002/diff/160001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:34: // It retrieves the handle for the syzyasan_rtl.dll module On 2017/08/08 15:11:55, Sébastien Marchand wrote: > This should go in the body of the function. Done. https://codereview.chromium.org/2984933002/diff/160001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:56: AsanRuntimePointers __attribute__((no_sanitize_address)) AsanRuntimePointers::asan_runtime; On 2017/08/08 15:11:55, Sébastien Marchand wrote: > This is > 80 characters. Done. https://codereview.chromium.org/2984933002/diff/160001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:56: AsanRuntimePointers __attribute__((no_sanitize_address)) AsanRuntimePointers::asan_runtime; On 2017/08/08 15:11:55, Sébastien Marchand wrote: > This is > 80 characters. Done. https://codereview.chromium.org/2984933002/diff/160001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:60: const AsanRuntimePointers* GetAsanHeapShim() { On 2017/08/08 15:11:55, Sébastien Marchand wrote: > Do we need this? Couldn't we use AsanRuntimePointers::asan_runtime directly? Done. https://codereview.chromium.org/2984933002/diff/160001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:100: // TODO(sebmarchand): Check if it is possible to use asan_heap here. On 2017/08/08 15:11:55, Sébastien Marchand wrote: > Remove this TODO, add something like this instead: > // The core CRT functions don't use the CRT's memory management > // functions, instead they directly use |__acrt_heap| and calls the > // ::Heap* functions. Because of this it's not possible to replace this > // heap by an Asan one. Done.
The CQ bit was checked by njanevsk@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm with a few nits https://codereview.chromium.org/2984933002/diff/180001/syzygy/integration_tes... File syzygy/integration_tests/allocator_shim.cc (right): https://codereview.chromium.org/2984933002/diff/180001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:54: static AsanRuntimePointers asan_runtime; static goes before the non static members. https://codereview.chromium.org/2984933002/diff/180001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:58: // cause that requireds additional methods from asan that are not supported s/cause/because/ s/requireds/requires/ s/asan/Asan/ https://codereview.chromium.org/2984933002/diff/180001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:59: // in the syzyasan runtime library. s/syzyasan/SyzyAsan/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Done! https://codereview.chromium.org/2984933002/diff/180001/syzygy/integration_tes... File syzygy/integration_tests/allocator_shim.cc (right): https://codereview.chromium.org/2984933002/diff/180001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:54: static AsanRuntimePointers asan_runtime; On 2017/08/08 18:12:32, Sébastien Marchand wrote: > static goes before the non static members. Done. https://codereview.chromium.org/2984933002/diff/180001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:58: // cause that requireds additional methods from asan that are not supported On 2017/08/08 18:12:32, Sébastien Marchand wrote: > s/cause/because/ > s/requireds/requires/ > s/asan/Asan/ Done. https://codereview.chromium.org/2984933002/diff/180001/syzygy/integration_tes... syzygy/integration_tests/allocator_shim.cc:59: // in the syzyasan runtime library. On 2017/08/08 18:12:32, Sébastien Marchand wrote: > s/syzyasan/SyzyAsan/ Done.
Done!
The CQ bit was checked by njanevsk@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by njanevsk@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sebmarchand@chromium.org Link to the patchset: https://codereview.chromium.org/2984933002/#ps200001 (title: "Response to reviewers comments.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1502225577641650,
"parent_rev": "df57cbafe7444b4f296e4218c2450dffe0f70ad4", "commit_rev":
"d0372eb9d3bbf7451a908762babbe8e9c08cccef"}
Message was sent while issue was closed.
Description was changed from ========== This is a prototype of the allocator shim. Currently it compiles and the integration tests compile and link and use the allocator shims. It's been tested that the methods from the shims are used not the ones ucrt. Other comments and questions on the design are welcome. BUG= ========== to ========== This is a prototype of the allocator shim. Currently it compiles and the integration tests compile and link and use the allocator shims. It's been tested that the methods from the shims are used not the ones ucrt. Other comments and questions on the design are welcome. BUG= Review-Url: https://codereview.chromium.org/2984933002 Committed: https://github.com/google/syzygy/commit/d0372eb9d3bbf7451a908762babbe8e9c08cccef ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://github.com/google/syzygy/commit/d0372eb9d3bbf7451a908762babbe8e9c08cccef |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
