http://bugs.winehq.org/show_bug.cgi?id=20759
Summary: Read buffer overflow in NdrConformantArrayMarshall? Product: Wine Version: 1.1.33 Platform: PC OS/Version: Linux Status: NEW Keywords: source, testcase Severity: normal Priority: P2 Component: rpc AssignedTo: wine-bugs@winehq.org ReportedBy: dank@kegel.com
http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/vg-ole32_marshal.txt contains a new warning thanks to the heap tail check:
Invalid read of size 1 at memcpy (mc_replace_strmem.c:482) by safe_copy_to_buffer (ndr_marshall.c:707) by array_write_variance_and_marshall (ndr_marshall.c:1926) by NdrConformantArrayMarshall (ndr_marshall.c:3626) by PointerMarshall (ndr_marshall.c:816) by NdrPointerMarshall (ndr_marshall.c:1488) by PointerMarshall (ndr_marshall.c:816) by NdrPointerMarshall (ndr_marshall.c:1488) by IRemUnknown_RemQueryInterface_Stub (dcom_p.c:386) by CStdStubBuffer_Invoke (cstub.c:475) by RPC_ExecuteCall (rpc.c:1392) by apartment_wndproc (compobj.c:885) by ??? (library.h:159) by call_window_proc (winproc.c:469) by WINPROC_CallProcAtoW (winproc.c:1023) by WINPROC_call_window (winproc.c:2225) by DispatchMessageA (message.c:3089) by host_object_proc (marshal.c:253) by ??? (signal_i386.c:2312) by call_thread_entry_point (signal_i386.c:2338) Address 0x7f04822f is 3 bytes after a block of size 44 alloc'd at notify_alloc (heap.c:279) by RtlAllocateHeap (heap.c:1521) by IMalloc_fnAlloc (ifs.c:186) by CoTaskMemAlloc (ifs.c:562) by RemUnknown_RemQueryInterface (stubmanager.c:657) by IRemUnknown_RemQueryInterface_Stub (dcom_p.c:370) by CStdStubBuffer_Invoke (cstub.c:475) by RPC_ExecuteCall (rpc.c:1392) by apartment_wndproc (compobj.c:885) by ??? (library.h:159) by call_window_proc (winproc.c:469) by WINPROC_CallProcAtoW (winproc.c:1023) by WINPROC_call_window (winproc.c:2225) by DispatchMessageA (message.c:3089) by host_object_proc (marshal.c:253) by ??? (signal_i386.c:2312) by call_thread_entry_point (signal_i386.c:2338) by start_thread (thread.c:469) by start_thread (pthread_create.c:297) by clone (clone.S:130)
This can be reproduced locally by setting up valgrind as described in http://wiki.winehq.org/Valgrind and applying the heap tail check patch to wine, starting winemine (to avoid valgrinding services), then running
WINETEST_PLATFORM=wine WINE_HEAP_REDZONE=16 valgrind --trace-children=yes --track-origins=yes --num-callers=30 wine ole32_test.exe.so marshal
(And, bonus deal, there's a null ptr crash in the same log file later down: Backtrace: =>0 test_local_server+0x5e4() [dlls/ole32/tests/marshal.c:2711] in ole32_test 1 func_marshal+0x1ab() [dlls/ole32/tests/marshal.c:3092] in ole32_test ... 2711 IClassFactory_Release(cf); but I suppose that might be a different bug.)
http://bugs.winehq.org/show_bug.cgi?id=20759
--- Comment #1 from Rob Shearman robertshearman@gmail.com 2009-11-20 17:15:41 --- The bug is in type_memsize in widl. The calculated size/alignment of REMQIRESULT/STDOBJREF in widl doesn't match that of the C compiler:
C:
fixme:ole:RemUnknown_RemQueryInterface sizeof(REMQIRESULT) = 44, __alignof__(REMQIRESULT) = 4 fixme:ole:RemUnknown_RemQueryInterface sizeof(STDOBJREF) = 40, __alignof__(STDOBJREF) = 4 fixme:ole:RemUnknown_RemQueryInterface sizeof(IPID) = 16, __alignof__(IPID) = 4 fixme:ole:RemUnknown_RemQueryInterface sizeof(OID) = 8, __alignof__(OID) = 8
widl:
sizeof(STDOBJREF) = 40, __alignof__(STDOBJREF) = 8 sizeof(REMQIRESULT) = 48, __alignof__(REMQIRESULT) = 8 sizeof(IPID) = 16, __alignof__(IPID) = 4 sizeof(OID) = 8, __alignof__(OID) = 8
The C standard doesn't help. From 6.7.2.1: "12 Each non-bit-field member of a structure or union object is aligned in an implementation- defined manner appropriate to its type."
http://bugs.winehq.org/show_bug.cgi?id=20759
--- Comment #2 from Alexandre Julliard julliard@winehq.org 2009-11-21 03:17:50 --- Probably hyper should be defined as INT64 or some similar type that has an explicit DECLSPEC_ALIGN(8). __int64 doesn't and should be avoided.
http://bugs.winehq.org/show_bug.cgi?id=20759
--- Comment #3 from Rob Shearman robertshearman@gmail.com 2009-11-23 09:13:16 --- I can confirm that Alexandre is correct - widl matches MIDL in the size and alignment of REMQIRESULT, so the problem is indeed in gcc's alignment of the hyper & MIDL_uhyper types not matching what we expect.
I have a patch in my tree that changes hyper to use INT64 instead of __int64, as Alexandre suggests, and this does indeed fix the discrepancy. I presume Alexandre will commit such a change later today.
However, there are other places in the source tree where __int64 is used and where the alignment will not be correct. These should be audited and fixed.
http://bugs.winehq.org/show_bug.cgi?id=20759
--- Comment #4 from Rob Shearman robertshearman@gmail.com 2009-11-23 09:23:14 --- Should have checked my mail before commenting. Everything that I suggested has already been committed: http://source.winehq.org/git/wine.git/?a=commit;h=a717d2d2840123872c17e00900... http://source.winehq.org/git/wine.git/?a=commit;h=7f69436ea874ad7c0b39f0f60e... http://source.winehq.org/git/wine.git/?a=commit;h=98de3950b1c1bc65d45d18e437...
http://bugs.winehq.org/show_bug.cgi?id=20759
Rob Shearman robertshearman@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |RESOLVED Resolution| |FIXED
--- Comment #5 from Rob Shearman robertshearman@gmail.com 2009-11-28 04:22:13 --- Warning no longer appears in new runs, therefore fixed: http://kegel.com/wine/valgrind/logs/2009-11-27-12.53/vg-ole32_marshal.txt
http://bugs.winehq.org/show_bug.cgi?id=20759
Alexandre Julliard julliard@winehq.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED
--- Comment #6 from Alexandre Julliard julliard@winehq.org 2009-12-04 12:16:38 --- Closing bugs fixed in 1.1.34.
-
wine-bugs＠winehq.org