http://bugs.winehq.org/show_bug.cgi?id=25186
Summary: Tom Clancy's Splinter Cell installer locks up Product: Wine Version: 1.3.7 Platform: x86 OS/Version: Linux Status: UNCONFIRMED Severity: normal Priority: P2 Component: shell32 AssignedTo: wine-bugs@winehq.org ReportedBy: pp@siedziba.pl
Splinter Cell installer enters infinite loop before it installs any files. Regression test finds this commit as the culprit:
f324f3c31ef4b0a6b5273aca534c173d68932462 is the first bad commit commit f324f3c31ef4b0a6b5273aca534c173d68932462 Author: Andrew Eikum aeikum@codeweavers.com Date: Thu Nov 11 15:58:12 2010 -0600
shell32: Don't fail if the path doesn't exist in Unix in IShellFolder::ParseDisplayName.
:040000 040000 319f1430fd21651467b6d173c38aa9b2ae2f25ca 97d7adfd2c2ad0c7b00aefed240ddf18a9899875 M dlls
Debugging the issue further, the patch introduces this loop:
1. while(!(pszUnixPath = wine_get_unix_file_name(dospath))){ 2. if(has_failed) 3. *dospath_end = '/'; 4. else 5. has_failed = 1; 6. while(*dospath_end != '\' && *dospath_end != '/') 7. --dospath_end; 8. *dospath_end = '\0'; 9. }
When I print the dospath variable between lines 1 and 2, the output is:
C:\Program Files\Ubi Soft Entertainment\Tom Clancy's Splinter Cell Demo\Support\Readme.txt C:\Program Files\Ubi Soft Entertainment\Tom Clancy's Splinter Cell Demo\Support C:\Program Files\Ubi Soft Entertainment\Tom Clancy's Splinter Cell Demo\Support C:\Program Files\Ubi Soft Entertainment\Tom Clancy's Splinter Cell Demo\Support [...]
... and it repeats ad infinitum.
The problem is, the inner while loop at line 6 immediately finds the '/' char written earlier at line 3, so dospath_end variable isn't decremented, and it loops forever.
I don't know what this piece of code is supposed to do, so I have no idea what is the correct way to fix it.
http://bugs.winehq.org/show_bug.cgi?id=25186
Piotr Pawlow pp@siedziba.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Keywords| |download, regression URL| |http://www.megagames.com/de | |mos/tom-clancys-splinter-ce | |ll-demo
--- Comment #1 from Piotr Pawlow pp@siedziba.pl 2010-11-15 19:45:40 CST --- Demo has the same problem, added download URL.
http://bugs.winehq.org/show_bug.cgi?id=25186
Austin English austinenglish@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |aeikum@codeweavers.com
http://bugs.winehq.org/show_bug.cgi?id=25186
GyB gyebro69@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |gyebro69@gmail.com
--- Comment #2 from GyB gyebro69@gmail.com 2010-11-17 11:59:11 CST --- The installer of Gunlok (an old, DX7 game) halts right after the copying of files begins. Regression testing resulted the same commit:
f324f3c31ef4b0a6b5273aca534c173d68932462 shell32: Don't fail if the path doesn't exist in Unix in IShellFolder::ParseDisplayName.
Still present in wine-1.3.7-144-gd7a0284.
http://bugs.winehq.org/show_bug.cgi?id=25186
--- Comment #3 from Andrew Eikum aeikum@codeweavers.com 2010-11-17 12:02:32 CST --- Yeah, this was a dumb mistake on my part. That section of the code tries to generate the most valid UNIX path possible, falling back on appending the unknown parts to the end of this partially-complete path. dospath_end should've been decremented after assigning its character to '/', to avoid this infinite loop. I've sent a patch that should fix this:
shell32: Fix an off-by-one error that causes an infinite loop http://source.winehq.org/patches/data/68602
Thanks for reporting, Piotr.
http://bugs.winehq.org/show_bug.cgi?id=25186
--- Comment #4 from Piotr Pawlow pp@siedziba.pl 2010-11-17 16:17:56 CST --- It works, thank you. However, there is one more thing that worries me:
01. while(!(pszUnixPath = wine_get_unix_file_name(dospath))){ 02. if(has_failed){ 03. *dospath_end = '/'; 04. --dospath_end; 05. }else 06. has_failed = 1; 07. while(*dospath_end != '\' && *dospath_end != '/'){ 08. --dospath_end; 09. if(dospath_end < dospath) 10. break; 11. } 12. *dospath_end = '\0'; 13. } 14. if(dospath_end < dospath) 15. return FALSE;
That "break" statement at line 10 will exit only the inner loop, right? With dospath_end being some random memory before dospath, it seems line 12 will clobber that random memory location with 0, then the test at line 1 will succeed again (because dospath hasn't really changed), and it will loop until it hits protected memory.
I wasn't able to test this scenario. The outer loop ends when it reaches the drive letter, it is assured by an earlier code which tests if the drive path exists. With the right timing I could probably remove the drive letter just at the right moment to make earlier test succeed but later test fail...
Anyway, maybe just remove lines 14 and 15, and replace that "break" at line 10 with "return FALSE"?
http://bugs.winehq.org/show_bug.cgi?id=25186
Andrew Eikum aeikum@codeweavers.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |RESOLVED Resolution| |FIXED
--- Comment #5 from Andrew Eikum aeikum@codeweavers.com 2010-11-23 09:36:44 CST --- You're right, although that code will probably never be tripped. I suspect I'll need to do more work on that area, and I'll queue a fix for it when I have a patch nearby. In the future, you can send a reply to the patch to wine-devel@winehq.org with your objection, which will help preventing bad code like that from being accepted. Thanks for looking closely.
Resolved by 2f05b5a6d981d54f272d193ea0bcd3a72144c88d.
http://bugs.winehq.org/show_bug.cgi?id=25186
Alexandre Julliard julliard@winehq.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED
--- Comment #6 from Alexandre Julliard julliard@winehq.org 2010-11-26 13:13:54 CST --- Closing bugs fixed in 1.3.8.
-
wine-bugs＠winehq.org