Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

i#4139: Add method to clear droption values for static re-attach #5335

Merged
merged 5 commits into from
Feb 10, 2022

Conversation

derekbruening
Copy link
Contributor

The values of droption accumulated options are not reset on detach,
causing them to accumulate in a statically-linked client.

There is no control point for droption to do this automatically, so we
add droption_parser_t::clear_values() that a client can call.

Adds a call to clear_values() in drmemtrace.

Augments the drmemtrace burst_malloc test to fail when the options are
not reset (i.e., fail without this fix). This serves as a test of the
new feature, as the existing droption tests do not have static clients
nor simple ways to add re-attaches.

Adds a section to the droption documentation explaining the new
feature.

Fixes #4139

The values of droption accumulated options are not reset on detach,
causing them to accumulate in a statically-linked client.

There is no control point for droption to do this automatically, so we
add droption_parser_t::clear_values() that a client can call.

Adds a call to clear_values() in drmemtrace.

Augments the drmemtrace burst_malloc test to fail when the options are
not reset (i.e., fail without this fix).  This serves as a test of the
new feature, as the existing droption tests do not have static clients
nor simple ways to add re-attaches.

Adds a section to the droption documentation explaining the new
feature.

Fixes #4139
@derekbruening
Copy link
Contributor Author

The windows crashes on burst_replaceall and burst_traceopts seem to be real problems (thought they were flakes at first). Looking.

clients/drcachesim/tracer/tracer.cpp Show resolved Hide resolved
clients/drcachesim/tracer/tracer.cpp Show resolved Hide resolved
@derekbruening
Copy link
Contributor Author

derekbruening commented Feb 9, 2022

The windows failure is a complex PEB state issue.

(3b2c.8f0): Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
tool_drcacheoff_burst_replaceall!droption_parser_t::allops+0x1a:
00007ff7`a36e51fa 488b0cca        mov     rcx,qword ptr [rdx+rcx*8] ds:00000000`00000000=????????????????
0:000> kn
 # Child-SP          RetAddr           Call Site
00 0000004d`625cf3c0 00007ff7`a36e5335 tool_drcacheoff_burst_replaceall!droption_parser_t::allops+0x1a [d:\derek\dr\git\build_x64_dbg_tests\ext\include\droption.h @ 383] 
01 0000004d`625cf3f0 00007ff7`a36e382e tool_drcacheoff_burst_replaceall!droption_parser_t::clear_values+0x15 [d:\derek\dr\git\build_x64_dbg_tests\ext\include\droption.h @ 354] 
02 0000004d`625cf480 00007ff7`a32e478e tool_drcacheoff_burst_replaceall!event_exit+0x41e [d:\derek\dr\git\src\clients\drcachesim\tracer\tracer.cpp @ 2005] 
03 0000004d`625cf540 00007ff7`a32c2ff0 tool_drcacheoff_burst_replaceall!instrument_exit_event+0xbe [d:\derek\dr\git\src\core\lib\instrument.c @ 866] 
04 0000004d`625cf5c0 00007ff7`a3559b20 tool_drcacheoff_burst_replaceall!dynamo_shared_exit+0x280 [d:\derek\dr\git\src\core\dynamo.c @ 1074] 
05 0000004d`625cf630 00007ff7`a32be3b8 tool_drcacheoff_burst_replaceall!detach_on_permanent_stack+0x1730 [d:\derek\dr\git\src\core\synch.c @ 2266] 
06 0000004d`625cfa50 00007ff7`a32be36b tool_drcacheoff_burst_replaceall!dr_app_stop_and_cleanup_with_stats+0x38 [d:\derek\dr\git\src\core\dynamo.c @ 2823] 
07 0000004d`625cfa80 00007ff7`a3256712 tool_drcacheoff_burst_replaceall!dr_app_stop_and_cleanup+0xb [d:\derek\dr\git\src\core\dynamo.c @ 2808] 
08 0000004d`625cfab0 00007ff7`a3266264 tool_drcacheoff_burst_replaceall!main+0x2c2 [d:\derek\dr\git\src\clients\drcachesim\tests\burst_replaceall.cpp @ 244] 
09 0000004d`625cfb00 00007ff7`a326618e tool_drcacheoff_burst_replaceall!invoke_main+0x34 [d:\agent\_work\2\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl @ 79] 
0a 0000004d`625cfb40 00007ff7`a326604e tool_drcacheoff_burst_replaceall!__scrt_common_main_seh+0x12e [d:\agent\_work\2\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl @ 288] 
0b 0000004d`625cfbb0 00007ff7`a32662d9 tool_drcacheoff_burst_replaceall!__scrt_common_main+0xe [d:\agent\_work\2\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl @ 331] 
0c 0000004d`625cfbe0 00007ffe`bd287034 tool_drcacheoff_burst_replaceall!mainCRTStartup+0x9 [d:\agent\_work\2\s\src\vctools\crt\vcstartup\src\startup\exe_main.cpp @ 17] 
0:000> ?? tool_drcacheoff_burst_replaceall!_tls_index
unsigned long 0
0:000> r
rax=0000000000000004 rbx=0000000000000000 rcx=0000000000000000
rdx=0000000000000000 rsi=0000004d625cf518 rdi=00007ff7a38e9c50
rip=00007ff7a36e51fa rsp=0000004d625cf3c0 rbp=0000000000000000
0:000> Uf tool_drcacheoff_burst_replaceall!droption_parser_t::allops
tool_drcacheoff_burst_replaceall!droption_parser_t::allops [d:\derek\dr\git\build_x64_dbg_tests\ext\include\droption.h @ 382]:
  382 00007ff7`a36e51e0 4883ec28        sub     rsp,28h
  383 00007ff7`a36e51e4 b804000000      mov     eax,4
  383 00007ff7`a36e51e9 8bc0            mov     eax,eax
  383 00007ff7`a36e51eb 8b0d979a1d00    mov     ecx,dword ptr [tool_drcacheoff_burst_replaceall!_tls_index (00007ff7`a38bec88)]
  383 00007ff7`a36e51f1 65488b142558000000 mov   rdx,qword ptr gs:[58h]
  383 00007ff7`a36e51fa 488b0cca        mov     rcx,qword ptr [rdx+rcx*8]
  383 00007ff7`a36e51fe 8b0408          mov     eax,dword ptr [rax+rcx]
  383 00007ff7`a36e5201 3905b1492000    cmp     dword ptr [tool_drcacheoff_burst_replaceall!TSS0<`template-parameter-2',droption_parser_t::llops,unsigned long,unsigned char &,void const std::vector<droption_parser_t * __ptr64,std::allocator<droption_parser_t * __ptr64> >::&,int, ?? &> (00007ff7`a38e9bb8)],eax
  383 00007ff7`a36e5207 7e39            jle     tool_drcacheoff_burst_replaceall!droption_parser_t::allops+0x62 (00007ff7`a36e5242)  Branch
0:000> dv
    allops_vec_ = { size=75 }
0:000> ?? &allops_vec_
class std::vector<droption_parser_t *,std::allocator<droption_parser_t *> > * 0x00007ff7`a38e9b98
   +0x000 _Mypair          : std::_Compressed_pair<std::allocator<droption_parser_t *>,std::_Vector_val<std::_Simple_types<droption_parser_t *> >,1>
0:000> ? @$teb
Evaluate expression: 332363919360 = 0000004d`626ef000
0:000> ?? heapmgt
struct _heap_management_t * 0x000001a5`d9d01050
   +0x000 vmheap           : vm_heap_t
   +0x0c8 vmcode           : vm_heap_t
   +0x190 dual_map_file    : (null) 
   +0x198 vmcode_writable_base : (null) 
   +0x1a0 vmcode_writable_alloc : (null) 
   +0x1a8 heap             : _heap_t
   +0x1c0 global_units     : _thread_units_t
   +0x640 global_nonpersistent_units : _thread_units_t
   +0xac0 global_heap_writable : 1 ''
   +0xac8 global_unprotected_units : _thread_units_t
   +0xf48 global_reachable_units : _thread_units_t
0:000> ?? heapmgt->vmheap
struct vm_heap_t
   +0x000 start_addr       : 0x000001a5`d9cc0000  ""
   +0x008 end_addr         : 0x000001a7`d9cc0000  "--- memory read error at address 0x000001a7`d9cc0000 ---"
   +0x010 alloc_start      : 0x000001a5`d9cc0000  ""
   +0x018 alloc_size       : 0x00000002`00001000
0:000> ?? heapmgt->vmcode
struct vm_heap_t
   +0x000 start_addr       : 0x00007ff7`23940000  ""
   +0x008 end_addr         : 0x00007ff7`43940000  "--- memory read erro

Hmm:

0:000> dt _TEB @$teb
tool_drcacheoff_burst_replaceall!_TEB
   +0x000 ExceptionList    : (null) 
   +0x008 StackBase        : 0x0000004d`625d0000 Void
   +0x010 StackLimit       : 0x0000004d`625cb000 Void
   +0x018 SubSystemTib     : (null) 
   +0x020 FiberData        : 0x00000000`00001e00 Void
   +0x020 Version          : 0x1e00
   +0x028 ArbitraryUserPointer : (null) 
   +0x030 Self             : 0x0000004d`626ef000 _TEB
   +0x038 EnvironmentPointer : (null) 
   +0x040 ClientId         : _CLIENT_ID
   +0x050 ActiveRpcHandle  : (null) 
   +0x058 ThreadLocalStoragePointer : (null) 

Xref #4002 VS2017 issues where static TLS support (#4030) was needed to
set up TEB.ThreadLocalStoragePointer in private libs, else this same
allops() had problems.

These tests use dr_app_stop_and_cleanup(): for which DR does
dispatch_enter_native() which does an fcache_enter to transfer control to
the target which is DR's dr_app_stop_and_cleanup function.
So that will swap the PEB fields to the app.
dispatch_enter_native does call dynamo_thread_not_under_dynamo but all that
does is turn off asynch interception: no PEB swap.
(You'd think it should do a PEB swap, right? But changing that function is
likely to cause big headaches trying to balance everything. Hmm.)
Then we run the detach_on_permanent_stack() w/ the fields as the app for
the main thread. dynamo_shared_exit() calls os_loader_exit() which does a
swap (again) to the app,
=>
So we should call swap_peb_pointer(my_dcontext, to-priv) in
detach_on_permanent_stack(), right? That seems to fix replaceall (still
has crash at very end but I think it did before): but traceopts now doesn't
crash but has an assert on the re-attach on TEB stack bounds.

Application D:\derek\dr\git\build_x64_dbg_tests\clients\bin64\tool.drcacheoff.burst_traceopts.exe (10964).  Internal Error: DynamoRIO debug check failure: D:\derek\dr\git\src\core\win32\os.c:5954 (get_allocation_base(stack_top - 1) == stack_base && (get_allocation_base(stack_top) != stack_base || is_wow64_process(NT_CURRENT_PROCESS))) || is_dynamo_address(stack_base)
05 000000a6`4d6fe810 00007ff7`795a83c0 tool_drcacheoff_burst_traceopts!d_r_internal_error+0x1a7 [d:\derek\dr\git\src\core\utils.c @ 193] 
06 000000a6`4d6fe990 00007ff7`795a5f92 tool_drcacheoff_burst_traceopts!get_stack_bounds+0x230 [d:\derek\dr\git\src\core\win32\os.c @ 5955] 
07 000000a6`4d6fe9e0 00007ff7`794a7d30 tool_drcacheoff_burst_traceopts!os_thread_init+0x42 [d:\derek\dr\git\src\core\win32\os.c @ 1674] 
08 000000a6`4d6fea20 00007ff7`794a5d56 tool_drcacheoff_burst_traceopts!dynamo_thread_init+0x9d0 [d:\derek\dr\git\src\core\dynamo.c @ 2381] 
09 000000a6`4d6feb30 00007ff7`794a54ce tool_drcacheoff_burst_traceopts!dynamorio_app_init_part_two_finalize+0x586 [d:\derek\dr\git\src\core\dynamo.c @ 670] 
0a 000000a6`4d6ff4b0 00007ff7`794a4f93 tool_drcacheoff_burst_traceopts!dynamorio_app_init+0xe [d:\derek\dr\git\src\core\dynamo.c @ 391] 
0b 000000a6`4d6ff4e0 00007ff7`7943697a tool_drcacheoff_burst_traceopts!dr_app_setup+0x83 [d:\derek\dr\git\src\core\dynamo.c @ 2734] 
0c 000000a6`4d6ff520 00007ff7`79436b2d tool_drcacheoff_burst_traceopts!gather_trace+0xba [d:\derek\dr\git\src\clients\drcachesim\tests\burst_traceopts.cpp @ 206] 
0d 000000a6`4d6ff5d0 00007ff7`794499e4 tool_drcacheoff_burst_traceopts!main+0xdd [d:\derek\dr\git\src\clients\drcachesim\tests\burst_traceopts.cpp @ 222] 
0e 000000a6`4d6ff960 00007ff7`7944990e tool_drcacheoff_burst_traceopts!invoke_main+0x34 [d:\agent\_work\2\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl @ 79] 
0f 000000a6`4d6ff9a0 00007ff7`794497ce tool_drcacheoff_burst_traceopts!__scrt_common_main_seh+0x12e [d:\agent\_work\2\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl @ 288] 
10 000000a6`4d6ffa10 00007ff7`79449a59 tool_drcacheoff_burst_traceopts!__scrt_common_main+0xe [d:\agent\_work\2\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl @ 331] 
11 000000a6`4d6ffa40 00007ffe`bd287034 tool_drcacheoff_burst_traceopts!mainCRTStartup+0x9 [d:\agent\_work\2\s\src\vctools\crt\vcstartup\src\startup\exe_main.cpp @ 17] 
12 000000a6`4d6ffa70 00007ffe`bd542651 KERNEL32!BaseThreadInitThunk+0x14
0:000> dv
       dcontext = 0x000001d2`8e674800
           base = 0x00000000`00000000
            top = 0x00000000`00000000
           ostd = 0x000001d2`8e697048
      stack_top = 0x000001ce`8e675000 "--- memory read error at address 0x000001ce`8e675000 ---"
     stack_base = 0x000000a6`4d600000 "--- memory read error at address 0x000000a6`4d600000 ---"
 dynamo_options = struct _options_t
      d_r_stats = <value unavailable>

@derekbruening
Copy link
Contributor Author

Filed #5340 on the windows issues.

Adds a missing swap to private TEB fields on
dr_app_stop_and_cleanup*(), and a swap back to app fields on detaching
thread cleanup.

This fixes problems using droption on client exit in drcachesim tests
for #4139.

Issue: #4139, #5340
Fixes #5340
@derekbruening
Copy link
Contributor Author

I'm thinking I'll separate out the #5340 fix into a separate PR; just waiting to make sure it fixes the droption issues here first.

@derekbruening derekbruening merged commit 4570a68 into master Feb 10, 2022
@derekbruening derekbruening deleted the i4139-droption-reset branch February 10, 2022 01:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DROPTION_FLAG_ACCUMULATE is not reset on detach
2 participants