[Bug 56661] New: Commit 8b3944e1341baaf693927c8b758851d2dbba725a breaks Project Diablo 2's Game.exe game client

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=56661

Bug ID: 56661
           Summary: Commit 8b3944e1341baaf693927c8b758851d2dbba725a breaks
                    Project Diablo 2's Game.exe game client
           Product: Wine
           Version: 9.8
          Hardware: x86-64
                OS: Linux
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: ntdll
          Assignee: [e-mail]
          Reporter: [e-mail]
      Distribution: ---

Commit 8b3944e1341baaf693927c8b758851d2dbba725a breaks Project Diablo 2's
Game.exe game client.

The game client itself crashes later in its error handling logic leading to a
stack overflow while trying to report the error

[user@monovo wine-git]$ ./wine ~/.wine/drive_c/Diablo2/ProjectD2/Game.exe -w
002c:err:winediag:getaddrinfo Failed to resolve your host name IP
0074:err:wineusb:DriverEntry Failed to initialize Unix library, status
0xc0000135.
0074:err:ntoskrnl:ZwLoadDriver failed to create driver
L"\Registry\Machine\System\CurrentControlSet\Services\wineusb": c0000135
003c:fixme:service:scmdatabase_autostart_services Auto-start service L"wineusb"
failed to start: 126
wine: Unhandled stack overflow at address 6FF6879B (thread 0024), starting
debugger...
0024:err:virtual:virtual_setup_exception stack overflow 1408 bytes addr
0x7bf966ef stack 0x220a80 (0x220000-0x221000-0x320000)

The game client itself works fine before this commit - discovered using git
bisect.

To setup PD2 follow the instructions at https://www.projectdiablo2.com/ For
starting the game client a set of battlenet cd keys is not necessary.

[Bug 56661] Project Diablo 2 crashes

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=56661

Zeb Figura  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
    Regression SHA1|                            |8b3944e1341baaf693927c8b758
                   |                            |851d2dbba725a
            Summary|Commit                      |Project Diablo 2 crashes
                   |8b3944e1341baaf693927c8b758 |
                   |851d2dbba725a breaks        |
                   |Project Diablo 2's Game.exe |
                   |game client                 |
           Keywords|                            |regression
                 CC|                            |[e-mail]

--- Comment #1 from Zeb Figura  ---
commit 8b3944e1341baaf693927c8b758851d2dbba725a
Author: Paul Gofman 
Date:   Thu Mar 7 16:52:30 2024 -0600

ntdll: Only allocate debug info in critical sections with
RTL_CRITICAL_SECTION_FLAG_FORCE_DEBUG_INFO.

[Bug 56661] Project Diablo 2 crashes

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=56661

--- Comment #2 from Paul Gofman  ---
Created attachment 76451
  --> https://bugs.winehq.org/attachment.cgi?id=76451
Compat shim on Windows

Indeed, the fatal stack overflow is in the mod's error reporting logic (and is
unrelated to the issue here), while the original error triggered in the Fog.dll
(one of the libraries shipped with the mod) is "FogValidateCriticalSection(%d)
- bad critical section err=%d". After initializing some critical section with
kernel32.InitializeCriticalSection() the dll performs some validation of the
critical section and explicitly expects DebugInfo allocated here. It first
checks this field for 0 (which succeds as it is -1 without crit sect debug
info), then '(DebugInfo & 3) == 0' and that fails trigerring the fatal error
(which is fatal regardless of the stack overflow, it is going to exit process
after anyway).

It is certain however that on Windows starting from Win8 the same '-1' is there
in DebugInfo field and the debug info is not allocated without
InitializeCriticalSectionEx with a special flag.

The game / mod work on modern Windows because of special compatibility shim. I
am attaching a screenshot from Windows Compatibility Administrator tool showing
that Windows compat database enables 'AllocDebugInfoForCritSections' hack. I.
e., on Windows the old behaviour is enabled on modern Windows specifically for
this game.

We don't have similar compat shim facilities currently. I am not sure if we
want to introduce something like that but so far looks like rather long shot to
me. What we have is setting Windows version in winecfg tool for the prefix or
specific application. In general binding different behaviour to this version
selection is not favoured in Wine, but we have a couple of places where it is
done (specifically, in kernel32/file.c and kernelbase/files.c to support
backward compatibility in a core part which changed in newer Windows versions.

[Bug 56661] Project Diablo 2 crashes

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=56661

--- Comment #3 from Paul Gofman  ---
I sent MR: https://gitlab.winehq.org/wine/wine/-/merge_requests/5635

With that the mod should work when setting Windows Version to Win7 or below in
winecfg. I am honestly not sure if that MR is going in but so far not sure if
there is any better way to fix such sort of regression.

[Bug 56661] Project Diablo 2 crashes

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=56661

--- Comment #4 from Nikolay Sivov  ---
I think it's fine as a regression fix, but having support for shims is better,
as unfortunate as they are.

[Bug 56661] Project Diablo 2 crashes

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=56661

Dmitry Timoshkov  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1

--- Comment #5 from Dmitry Timoshkov  ---
(In reply to Nikolay Sivov from comment #4)
> I think it's fine as a regression fix, but having support for shims is
> better, as unfortunate as they are.

Shims is a part of Windows OS and is copyrighted material. Even if Wine has
support for shims at some point how are you planning to integrate Windows shims
database into Wine?

[Bug 56661] Project Diablo 2 crashes

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=56661

--- Comment #6 from Paul Gofman  ---
(In reply to Dmitry Timoshkov from comment #5)
> (In reply to Nikolay Sivov from comment #4)
> > I think it's fine as a regression fix, but having support for shims is
> > better, as unfortunate as they are.
>
> Shims is a part of Windows OS and is copyrighted material. Even if Wine has
> support for shims at some point how are you planning to integrate Windows
> shims database into Wine?

I think implementing same mechanics doesn't not necessarily assume copying
Windows database, or even supporting the format (while the latter would be
ideal of course because I saw some games shipping and installing own shim DB
with them). Implementing that would probably mean adding compat hacks manually.

[Bug 56661] Project Diablo 2 crashes

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=56661

--- Comment #7 from Dmitry Timoshkov  ---
(In reply to Paul Gofman from comment #6)
> I think implementing same mechanics doesn't not necessarily assume copying
> Windows database, or even supporting the format (while the latter would be
> ideal of course because I saw some games shipping and installing own shim DB
> with them). Implementing that would probably mean adding compat hacks
> manually.

Adding compatibility hacks manually implies using Windows shims database as a
reference, which seems to be a copyright violation.

[Bug 56661] Project Diablo 2 crashes

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=56661

--- Comment #8 from Paul Gofman  ---
(In reply to Dmitry Timoshkov from comment #7)
> (In reply to Paul Gofman from comment #6)
> > I think implementing same mechanics doesn't not necessarily assume copying
> > Windows database, or even supporting the format (while the latter would be
> > ideal of course because I saw some games shipping and installing own shim DB
> > with them). Implementing that would probably mean adding compat hacks
> > manually.
>
> Adding compatibility hacks manually implies using Windows shims database as
> a reference, which seems to be a copyright violation.

How is that different from, e. g, looking at MSVS headers looking at structure
definition and function prototypes? The database is available to view with a MS
GUI tool (from which I attached a screenshot), without looking at any internal
MS files. Also subject for unit testing.

[Bug 56661] Project Diablo 2 crashes

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=56661

--- Comment #9 from Dmitry Timoshkov  ---
(In reply to Paul Gofman from comment #8)
> (In reply to Dmitry Timoshkov from comment #7)
> > (In reply to Paul Gofman from comment #6)
> > > I think implementing same mechanics doesn't not necessarily assume copying
> > > Windows database, or even supporting the format (while the latter would be
> > > ideal of course because I saw some games shipping and installing own shim DB
> > > with them). Implementing that would probably mean adding compat hacks
> > > manually.
> >
> > Adding compatibility hacks manually implies using Windows shims database as
> > a reference, which seems to be a copyright violation.
>
> How is that different from, e. g, looking at MSVS headers looking at
> structure definition and function prototypes? The database is available to
> view with a MS GUI tool (from which I attached a screenshot), without
> looking at any internal MS files. Also subject for unit testing.

MSVC headers are explicitly designed and licensed to be used in the source
form. Inspecting an internal Windows binary database even with the special tool
doesn't grant a license for using it in a different product, even in a
different format.

[Bug 56661] Project Diablo 2 crashes

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=56661

Zeb Figura  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #10 from Zeb Figura  ---
(In reply to Dmitry Timoshkov from comment #9)
> MSVC headers are explicitly designed and licensed to be used in the source
> form.

Where are you getting that? Most of them say "all rights reserved"; I'm not
aware of any license which explicitly allows for rewriting them with compatible
contents?

And perhaps more saliently, what about the registry, then? Or NLS files? What
makes this case different?

[Bug 56661] Project Diablo 2 crashes

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=56661

Austin English  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

[Bug 56661] Project Diablo 2 crashes

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=56661

--- Comment #11 from Dmitry Timoshkov  ---
(In reply to Zeb Figura from comment #10)
> (In reply to Dmitry Timoshkov from comment #9)
> > MSVC headers are explicitly designed and licensed to be used in the source
> > form.
>
> Where are you getting that? Most of them say "all rights reserved"; I'm not
> aware of any license which explicitly allows for rewriting them with
> compatible contents?

I'm not sure where did you get an idea about rewriting.
> And perhaps more saliently, what about the registry, then? Or NLS files?
> What makes this case different?

Yes, it's basically the same reason why we can't get a random binary file from
Windows, decompile it, and then re-use its contents in Wine.

[Bug 56661] Project Diablo 2 crashes

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=56661

Paul Gofman  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED
      Fixed by SHA1|                            |3c0e318582f1b3058ab3fc8f858
                   |                            |f6a0edacc59ad

--- Comment #12 from Paul Gofman  ---
The old critical section behaviour is restored now (in the just updated Wine
git) if Windows version set to Win7 or below in winecfg. Stanislav, could you
please confirm if the game works again this way?

[Bug 56661] Project Diablo 2 crashes

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=56661

--- Comment #13 from Stanislav Paskalev  ---
Thank you Paul and team, I confirm that the game starts correctly now when I
select Windows 7 via winecfg.

[Bug 56661] Project Diablo 2 crashes

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=56661

--- Comment #14 from Austin English  ---
(In reply to Nikolay Sivov from comment #4)
> I think it's fine as a regression fix, but having support for shims is
> better, as unfortunate as they are.

Since this bug is targeted specifically to Project Diablo 2, I opened bug 56680
specifically for application shim support.

[Bug 56661] Project Diablo 2 crashes

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=56661

Alexandre Julliard  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #15 from Alexandre Julliard  ---
Closing bugs fixed in 9.9.