https://bugs.winehq.org/show_bug.cgi?id=36731
Bug ID: 36731 Summary: Runes of Magic 'ClientUpdater.exe' crashes after a number of update cycles (mshtml environment setup contains stack buffer overflow) Product: Wine Version: 1.7.20 Hardware: x86 OS: Linux Status: NEW Severity: normal Priority: P2 Component: mshtml Assignee: wine-bugs@winehq.org Reporter: focht@gmx.net
Hello folks,
found during investigation of 'Runes of Magic' client updater.
There is a crash after a number of update cycles.
Unfortunately it's not easily traceable as it requires large downloads and many client restarts (= hours) to reach the crash point.
I started the updater with a few debug channels (= reduced noise) which still allowed me to do post-mortem analysis.
The launcher restarts itself after each update cycle.
--- snip --- $ pwd /home/focht/.wine/drive_c/Program Files/Runes of Magic
$ WINEDEBUG=+tid,+seh,+loaddll,+process,+mshtml wine ./launcher.exe ... <hours, multiple updater restarts> ... 004f:trace:loaddll:load_builtin_dll Loaded L"C:\windows\system32\mshtml.dll" at 0x7c090000: builtin 004f:trace:mshtml:DllGetClassObject (CLSID_HTMLDocument {00000001-0000-0000-c000-000000000046} 0x3392a8) 004f:trace:mshtml:ClassFactory_AddRef (0x1c2c80) ref = 1 004f:trace:mshtml:HTMLDocument_Create ((nil) IID_IUnknown 0x1c19d4) 004f:trace:mshtml:load_gecko () 004f:trace:mshtml:check_version "Wine Gecko 2.24" 004f:trace:mshtml:load_xul (L"C:\windows\system32\gecko\2.24\wine_gecko\\xul.dll") 004f:trace:seh:raise_exception code=c0000005 flags=0 addr=0x7c130001 ip=7c130001 tid=004f 004f:trace:seh:raise_exception info[0]=00000001 004f:trace:seh:raise_exception info[1]=8d43ade4 004f:trace:seh:raise_exception eax=00000001 ebx=006b0063 ecx=003389a0 edx=7bcda204 esi=00339330 edi=001c2de8 004f:trace:seh:raise_exception ebp=005c0070 esp=00338c00 cs=0023 ds=002b es=002b fs=0063 gs=006b flags=00010212 004f:trace:seh:call_stack_handlers calling handler at 0x4a97b0 code=c0000005 flags=0 004f:trace:seh:call_stack_handlers handler at 0x4a97b0 returned 1 004f:trace:seh:call_stack_handlers calling handler at 0x7bc9ecf7 code=c0000005 flags=0 wine: Unhandled page fault on write access to 0x8d43ade4 at address 0x7c130001 (thread 004f), starting debugger... --- snip ---
I looked at the crash site and noticed it being in the middle of opcode sequence.
--- snip --- 7C130000 45 INC EBP 7C130001 0889 4424108D OR BYTE PTR DS:[ECX+8D102444],CL 7C130007 8313 B5 ADC DWORD PTR DS:[EBX],-4B 7C13000A FA CLI --- snip ---
Decoded with proper opcode start addresses:
--- snip --- 7C12FFFF 8B45 08 MOV EAX,DWORD PTR SS:[EBP+8] 7C130002 894424 10 MOV DWORD PTR SS:[ESP+10],EAX 7C130006 8D83 13B5FAFF LEA EAX,[EBX+FFFAB513] --- snip ---
Partial stack dump with 'esp' = 0x00338c00 from exception context:
--- snip --- ... 00338BA8 C0000005 00338BAC 00000000 00338BB0 00000000 00338BB4 7C130001 00338BB8 00000002 00338BBC 00000001 00338BC0 8D43ADE4 äC 00338BC4 00650074 t e 00338BC8 0033006D m 3 00338BCC 005C0032 2 \ 00338BD0 00650067 g e 00338BD4 006B0063 c k 00338BD8 005C006F o \ 00338BDC 002E0032 2 . 00338BE0 00340032 2 4 00338BE4 0077005C \ w 00338BE8 006E0069 i n 00338BEC 00000005 00338BF0 00650067 g e 00338BF4 006B0063 c k 00338BF8 005C006F o \ 00338BFC 7C130000 00338C00 00338F74 ; UNICODE "C:\windows\system32\gecko\2.24\wine_gecko" 00338C04 7C1FF480 00338C08 7C19E234 ; ASCII "load_xul" 00338C0C 7C19CF48 ; ASCII "(%s)" 00338C10 7BCEC8C1 ; ASCII "L"C:\windows\system32\gecko\2.24\wine_gecko\\xul.dll"" 00338C14 7C19D4E8 ; ASCII "Wine Gecko 2.24" 00338C18 7C19E250 ; ASCII "check_version" 00338C1C 7C19D4C0 ; ASCII "%s" 00338C20 7BCEC8AF ; ASCII ""Wine Gecko 2.24"" ... --- snip ---
Yes, looks pretty much like a stack buffer overflow. A string buffer overwrote 'ebp', 'ebx' values (register save on stack for '__x86_get_pc_thunk_bx') and parts of the return address. The NULL terminator cancelled out the lower 16 bits of the return address.
The culprit: 'load_xul' -> 'set_environment'
Source: http://source.winehq.org/git/wine.git/blob/0be56d27d2d4b22367313fa4c6f1e6586...
--- snip --- 439 static void set_environment(LPCWSTR gre_path) 440 { 441 WCHAR path_env[MAX_PATH], buf[20]; 442 int len, debug_level = 0; 443 444 static const WCHAR pathW[] = {'P','A','T','H',0}; 445 static const WCHAR warnW[] = {'w','a','r','n',0}; 446 static const WCHAR xpcom_debug_breakW[] = 447 {'X','P','C','O','M','_','D','E','B','U','G','_','B','R','E','A','K',0}; 448 static const WCHAR nspr_log_modulesW[] = 449 {'N','S','P','R','_','L','O','G','_','M','O','D','U','L','E','S',0}; 450 static const WCHAR debug_formatW[] = {'a','l','l',':','%','d',0}; 451 452 /* We have to modify PATH as XPCOM loads other DLLs from this directory. */ 453 GetEnvironmentVariableW(pathW, path_env, sizeof(path_env)/sizeof(WCHAR)); 454 len = strlenW(path_env); 455 path_env[len++] = ';'; 456 strcpyW(path_env+len, gre_path); 457 SetEnvironmentVariableW(pathW, path_env); 458 459 SetEnvironmentVariableW(xpcom_debug_breakW, warnW); 460 461 if(TRACE_ON(gecko)) 462 debug_level = 5; 463 else if(WARN_ON(gecko)) 464 debug_level = 3; 465 else if(ERR_ON(gecko)) 466 debug_level = 2; 467 468 sprintfW(buf, debug_formatW, debug_level); 469 SetEnvironmentVariableW(nspr_log_modulesW, buf); 470 } --- snip ---
'path_env' must have overflowed ... but how?
I used a JIT debugger to examine the process environment block at the time of the crash since 'GetEnvironmentVariableW' reads from 'NtCurrentTeb()->Peb->ProcessParameters->Environment'.
--- snip ---- Address UNICODE dump ... 00231EC0 m32\cmd.exe.PATH 00231EE0 =C:\windows\syst 00231F00 em32;C:\windows; 00231F20 C:\windows\syste 00231F40 m32\wbem;C:\wind 00231F60 ows\system32\gec 00231F80 ko\2.24\wine_gec 00231FA0 ko;C:\windows\s 00231FC0 ystem32\gecko\2. 00231FE0 24\wine_gecko;C 00232000 :\windows\system 00232020 32\gecko\2.24\wi 00232040 ne_gecko;C:\win 00232060 dows\system32\ge 00232080 cko\2.24\wine_ge 002320A0 cko;C:\windows\ 002320C0 system32\gecko\2 002320E0 .24\wine_gecko. 00232100 TEMP=C:\users\fo 00232120 cht\Temp.TMP=C:\ 00232140 users\focht\Temp 00232160 .windir=C:\windo 00232180 ws.ALLUSERSPROFI 002321A0 LE=C:\users\Publ 002321C0 ic.APPDATA=C:\us ... --- snip ---
At the time 'gre_path' path was appended, the string from 'PATH' environment variable had already grown near 'MAX_PATH' (260 characters) buffer limit.
'PATH' is of course not limited to 'MAX_PATH' since it contains a list of paths. A better option would be to query with 'GetEnvironmentVariableW( value, NULL, 0)' first and allocate the needed buffer from heap, including length for 'gre_path'.
Even with these things corrected there is still a general problem: at one point it will overflow/being blocked from appending to 'PATH'. Each newly created updater process inherits the process environment from parent (client updater restarts itself each time). A more sophisticated thing to do would be to search for existing value and not append if already present.
Wine Mono 'mscoree' component has a similar potential stack buffer overflow:
http://source.winehq.org/git/wine.git/blob/8cdcf470016f0655dfc8810f9d4d2f2d7...
Regards