[PATCH 0/2] MR9711: msado15: Remove left over free in get_accessor (ASan).
The first patch removes an unconditional `free( key )`, which holds here a pointer to the stack variable `tmp`. I assume this is a left over, because the next line outside the block is again a `free( key )`, this time with condition. The second patch adds a return in case the `malloc` of `elem` failed. Are both of my assumptions right here? Followup to f2ea57b7b7. CC: @piotr [ASan report in gitlab](https://gitlab.winehq.org/bernhardu/wine/-/jobs/215621#L6107) <details> <summary>ASan details</summary> ``` (lldb) bt * thread #1, stop reason = breakpoint 1.4 * frame #0: 0x79e8b843 libclang_rt.asan_dynamic-i386.dll`ReportError at asan_report.cpp:227:29 [inlined] frame #1: 0x79e8b843 libclang_rt.asan_dynamic-i386.dll`::ReportFreeNotMalloced() at asan_report.cpp:269:13 frame #2: 0x79e68da4 libclang_rt.asan_dynamic-i386.dll`::Deallocate() at asan_allocator.cpp:0 frame #3: 0x79e66cb8 libclang_rt.asan_dynamic-i386.dll`__ZN6__asan9asan_freeEPvPN11__sanitizer18BufferedStackTraceE at asan_allocator.cpp:1017:12 frame #4: 0x79e8520c libclang_rt.asan_dynamic-i386.dll`::free() at asan_malloc_win.cpp:72:10 frame #5: 0x6e9ab979 msado15.dll`get_accessor(recordset=0x03403970, fields=0x0059f8f0, hacc=0x0059f940) at recordset.c:2280 frame #6: 0x6e9a11e9 msado15.dll`_recordset_AddNew(a)36(iface=<unavailable>, field_list=<unavailable>, values=<unavailable>) at recordset.c:2326 frame #7: 0x0041107a msado15_test.exe`_Recordset_AddNew(This=<unavailable>, field_list=<unavailable>, values=<unavailable>) at msado15_backcompat.h:5779 frame #8: 0x00405c01 msado15_test.exe`test_ADORecordsetConstruction(exact_scroll=<unavailable>) at msado15.c:2061 frame #9: 0x00401038 msado15_test.exe`func_msado15 at msado15.c:3221 frame #10: 0x0041a662 msado15_test.exe`run_test(name="msado15") at test.h:780 frame #11: 0x0041a17c msado15_test.exe`main(argc=<unavailable>, argv=<unavailable>) at test.h:0 frame #12: 0x0041be56 msado15_test.exe`mainCRTStartup at crt_main.c:62 frame #13: 0x7bd33690 kernel32.dll`BaseThreadInitThunk + 16 frame #14: 0x7be667ab ntdll.dll`_call_thread_func_wrapper + 23 frame #15: 0x7be6711b ntdll.dll`call_thread_func(entry=(msado15_test.exe`mainCRTStartup at crt_main.c:45), arg=0x7ffd1000) at signal_i386.c:503 (lldb) frame select 5 frame #5: 0x6e9ab979 msado15.dll`get_accessor(recordset=0x03403970, fields=0x0059f8f0, hacc=0x0059f940) at recordset.c:2280 2277 memcpy( elem->key.data, key->data, key->len * sizeof(key->data[0]) ); 2278 rb_put( &recordset->hacc_cache, key, &elem->entry ); 2279 } -> 2280 free( key ); 2281 } 2282 if ((BYTE *)key != tmp) free( key ); 2283 (lldb) print key (hacc_cache_key *) 0x0059f6c0 (lldb) print (char*)tmp (char *) 0x0059f6c0 "" (lldb) ================================================================= ==msado15_test.exe==1092==ERROR: AddressSanitizer: attempting free on address which was not malloc()-ed: 0x0059f6c0 in thread T0 #0 0x79e851bb in free /home/runner/work/llvm-mingw/llvm-mingw/llvm-project/compiler-rt\lib/asan/asan_malloc_win.cpp:71:3 #1 0x6e9ab978 in get_accessor .../wine/dlls/msado15/recordset.c:2280 #2 0x6e9a11e8 in recordset_AddNew .../wine/dlls/msado15/recordset.c:2326 #3 0x00411079 in _Recordset_AddNew .../obj\include\msado15_backcompat.h:5779 #4 0x00405c00 in test_ADORecordsetConstruction .../wine/dlls/msado15/tests/msado15.c:2061 #5 0x00401037 in func_msado15 .../wine/dlls/msado15/tests/msado15.c:3221 #6 0x0041a661 in run_test .../wine/include/wine/test.h:780 #7 0x0041a17b in main .../wine/include/wine/test.h #8 0x0041be55 in mainCRTStartup .../wine/dlls/msvcrt/crt_main.c:62 #9 0x7bd3368f in BaseThreadInitThunk (C:\windows\system32\kernel32.dll+0x7b82368f) #10 0x7be667aa in call_thread_func_wrapper (C:\windows\system32\ntdll.dll+0x7bc467aa) #11 0x7be6711a in call_thread_func .../wine/dlls/ntdll/signal_i386.c:503 Address 0x0059f6c0 is located in stack of thread T0 at offset 32 in frame #0 0x6e9aac2f in get_accessor .../wine/dlls/msado15/recordset.c:2179 This frame has 2 object(s): [16, 20) 'refcount' (line 2182) [32, 288) 'tmp' (line 2183) <== Memory access at offset 32 is inside this variable HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork (longjmp, SEH and C++ exceptions *are* supported) SUMMARY: AddressSanitizer: bad-free .../wine/dlls/msado15/recordset.c:2280 in get_accessor ``` </details> -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9711
From: Bernhard Übelacker <bernhardu(a)mailbox.org> Followup to f2ea57b7b7. --- dlls/msado15/recordset.c | 1 - 1 file changed, 1 deletion(-) diff --git a/dlls/msado15/recordset.c b/dlls/msado15/recordset.c index 67f9fff3188..3d04f4508d2 100644 --- a/dlls/msado15/recordset.c +++ b/dlls/msado15/recordset.c @@ -2277,7 +2277,6 @@ static HRESULT get_accessor( struct recordset *recordset, VARIANT *fields, HACCE memcpy( elem->key.data, key->data, key->len * sizeof(key->data[0]) ); rb_put( &recordset->hacc_cache, key, &elem->entry ); } - free( key ); } if ((BYTE *)key != tmp) free( key ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9711
From: Bernhard Übelacker <bernhardu(a)mailbox.org> Followup to f2ea57b7b7. --- dlls/msado15/recordset.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/msado15/recordset.c b/dlls/msado15/recordset.c index 3d04f4508d2..d47465da936 100644 --- a/dlls/msado15/recordset.c +++ b/dlls/msado15/recordset.c @@ -2254,6 +2254,7 @@ static HRESULT get_accessor( struct recordset *recordset, VARIANT *fields, HACCE { if ((BYTE *)key != tmp) free( key ); free( bindings ); + return E_OUTOFMEMORY; } for (i = 0; i < key->len; i++) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9711
This merge request was approved by Piotr Caban. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9711
participants (2)
-
Bernhard Übelacker -
Piotr Caban (＠piotr)