narzoul / DDrawCompat

DirectDraw and Direct3D 1-7 compatibility, performance and visual enhancements for Windows Vista, 7, 8, 10 and 11
BSD Zero Clause License
878 stars 67 forks source link

C&C Tiberian Sun crashes from lasers, Disruptor beams and Ion Cannon impacts #289

Closed Bittah closed 3 months ago

Bittah commented 3 months ago

When a laser, Disruptor beam or the ripple effect from an Ion Canon's impact get clipped by the bottom of the screen while the camera view is moving upwards, the game crashes if the game is running at a 16:9 screen resolution such as 1280x720; this doesn't happen at 800x600 or 640x480.

I created a test map that allows you to quickly reproduce the crash: WCEtest.zip

The game will crash within a few seconds after the map has finished loading.

I also confirmed that the game doesn't crash when using cnc-ddraw, so maybe the fix could be ported over?

narzoul commented 3 months ago

Temporary fix: ddraw.zip (diff.txt compared to v0.5.1)

It's a bug is in the game engine. It reads/writes past the end of the surface memory that holds the current view's pixels. In case of 1280x720, it's a 1112x720 surface, which requires 1112x720x2 = 1601280 bytes of memory. DDrawCompat allocates an extra 32 bytes on top of that to mimic native ddraw's allocations (exactly to fix a similar bug in Diablo 1), which brings it to 1601312 bytes. With allocations of this size, even if it gets rounded up to the next page boundary (multiple of 4096), it will only be guarded up to 1601536 bytes, which is less than 1 row of extra "pixels".

I'm not sure how far the game goes out of bounds, but adding 1 extra row to the allocation size seems to "fix" this particular case. I'll probably add it as some configurable option in the next release, because I don't want to waste memory like that for every other well-behaving game. From a quick look at cnc-ddraw, it has a similar fix ("guard_lines" setting).

Bittah commented 3 months ago

I gave it a try and while the game no longer immediately crashes after loading the map, it does still crash when I let the game go on for a while longer. The crash always happens when the camera view reaches the bottom of the screen for the 3rd time. This is what's visible on the screen after the crash happens: image

Here's the except.txt generated by the crash, in case that's useful to you: except.txt

As the screenshot suggests, the Ion Cannon's ripple effect is responsible for the crash and I verified this by disabling the Ion Cannons on the map and seeing whether it'd still crash, which it didn't.

To make testing a bit quicker I made some adjustments to the test map: WCEtest_IC_only.zip The map now only has the Ion Cannons and starts scrolling up from the bottom. The game will crash as soon as the bottom of the screen clips through the middle of an Ion Cannon ripple effect, as shown in this screenshot: image

Bittah commented 3 months ago

I was going to post this as a separate issue, but considering that the temporary fix you posted above appears to positively affect Fog of War, I figured that it'd be most convenient to just comment here instead.

It's well known that enabling Fog of War is likely to crash the game, but similar to crashes from lasers, Disruptor Beams and Ion Cannon ripple effects, this appears to have been fixed in cnc-ddraw. Fog of War still crashes the game with DDrawCompat and it appears to happen with any resolution above 640x480 (so it even happens with 800x600).

I created a map to easily test this: FoWtest.zip

With the latest release of DDrawCompat the game will crash in about 2 seconds after the map has loaded at game speed 5 (60 FPS). This is visible on the screen when the crash happens: image

When I apply the temporary fix that you posted above however, it takes about 22 seconds at game speed 5, so it undeniably has an impact.

narzoul commented 3 months ago

Yeah, one row was not enough. I increased it to 200 by default for now (like in cnc-ddraw) and added a SurfaceExtraRows setting to allow changing it up to 1000, but I doubt that it's needed. Probably 200 is way overkill too.

ddraw.zip (diff.txt compared to v0.5.1)

It won't fix the delayed fog of war crash. That one seems to be a bit different issue. It's still an access violation error, but the erroneous access is more than 20 MB away from any ddraw surface memory, so it seems to be more than a simple buffer overflow error.

I get the same crash with the latest cnc-ddraw (6.3.0), around the same timing and at the exact same location in game.exe. Did you change some of its default settings to fix it?

narzoul commented 3 months ago

As suspected, the delayed crash is an entirely different issue. It affects an allocation created directly by game.exe through HeapAlloc. The allocation size is 1112x704x2, which is similar to the one used for the DirectDraw surface, except it's missing 16 rows, presumably to account for the top bar above the game map. The problem is that the game tries to render to those 16 missing rows too, which are not covered by the allocation.

I've made a hacky workaround just for testing purposes, but I don't indent to add it to an official release. This should be patched somehow in the executable. The workaround hooks HeapAlloc and HeapFree in the IAT, allocates some extra memory for all allocations that are large enough to potentially be used as a surface like the above (with minimum supported resolution of 640x480 in mind), and offsets the returned allocation by the extra size, so that the game can write in front of the returned pointer.

You can test it here (the previous "fix" is also included): ddraw.zip (diff.txt compared to v0.5.1)

Bittah commented 3 months ago

I already tested the fix from your comment from 19 hours ago and for me it actually already entirely fixes both the crashes from the Ion Cannon ripple effects and the crashes from Fog of War.

I was actually in the middle of tweaking the map a bit to see if I could get the game to crash from FoW again with the fix you posted before I noticed your last post, but I haven't been able to (I also tried it with cnc-ddraw to be sure, but it won't crash with that for me either).

It's known that Fog of War crashes happen more on certain systems than others though, but that does make it difficult for me to do more tests myself. I did try the workaround from your last comment to be sure though and as expected, the game also doesn't crash with that for me.

narzoul commented 3 months ago

It's possible that there is another allocation right in front of the critical one, which absorbs the invalid writes, though this means some other allocation is getting corrupted in the process.

I modified the previous workaround to test for this specifically. Now it not only allocates extra pages in front of the large allocations, but also removes read/write access from those pages. So if the invalid access does happen, it should definitely crash, and won't get absorbed by another allocation.

ddraw.zip (diff.txt compared to v0.5.1)

However, even with this I "only" get a crash 8/10 times, so there must be some other (timing?) element to it. Probably the test run is not entirely deterministic in terms of what happens on the map. It also only seems to crash at 720p resolutions like 1280x720 or 960x720 (so not only 16:9). I couldn't get a crash yet with different heights.

If it does crash, it always happens at a similar (but not entirely identical) point, shortly after the scrolling bounces back from the bottom for the first time. I find it faster to reproduce the issue by letting the game run at unlimited framerates. It doesn't seen to affect the approximate point at which the crash can happen, but it gets there significantly faster.

But, I'm also pretty certain at the point that the issue is unrelated to both DDrawCompat and DirectDraw in general, so I'll probably let it rest for now and focus on the actual DDrawCompat issues. The earlier workaround fixes the crash for me 10/10 times.

Bittah commented 3 months ago

While this last workaround initially wouldn't crash for me either, I now got it to crash pretty consistently at 1280x720 and the crash appears to happen at the same time as when it does for you. As a sanity check I then reverted to your earlier work-around and it stopped crashing again, so it seems like a good enough solution. Thanks for your work.

Metadorius commented 3 months ago

@narzoul do you have a commit hash or w/e which contains the change that fixes that? I am an engine extension author for YR and involved with CnCNet patches, I think it would be a good idea to get the fix working on engine level (too).

narzoul commented 3 months ago

It's in diff.txt. I always attach it next to the patch. Just "git apply" it on top of v0.5.1.

You can test it here (the previous "fix" is also included): ddraw.zip (diff.txt compared to v0.5.1)