Crash when exiting a multiplayer game

[Player701]

Recently, I've had GZDoom crash on me a few times when testing my ZScript code in multiplayer. However, it seems that the problem is not related to my code at all, as I've managed to get a crash without any PWADs loaded (except for brightmaps.pk3 and lights.pk3, but I doubt they matter). All I had to do was to run two instances of GZDoom at the same time and connect them to each other. The first instance are passed the following command-line parameters:

Code: Select all

gzdoom -host 2 -nomonsters +deathmatch 1 +sv_cheats 1

The second instance gets the following parameters:

Code: Select all

gzdoom -join localhost

The IWAD used is Doom II. After the connection is established and the actual game begins, I simply close one of the instances. I think it's the server one (as I get the message "Player is the new arbitrator" in the instance that I leave running). When it closes, BOOM - crash.

The place in the code where the crash happens is in dobjgc.h, line 99:

---

Unfortunately, the crash doesn't happen all the time. So far I haven't found out what exactly I have to do to ensure it always happens. I just run the game in the configuration I've described above and exit immediately. Sometimes it crashes, sometimes it does not. The version I'm using for these tests is g3.7pre-263-g45ef7bca4.

Attempting to bisect the repository to find out the commit where the crash was first introduced did not succeed - I could not find a good commit to start from. This commit is the oldest one I know is affected. Earlier versions either do not run due to script errors, or cannot be compiled outright due to Visual Studio throwing an internal compiler error when compiling zcc_expr.cpp. Even older versions require FMOD and similar stuff, which I don't have on my system.

I can provide a crash report if need be, but I'm not sure a crash report from a self-compiled debug build will be useful. Still, I can provide further feedback on request.

[drfrag]

or cannot be compiled outright due to Visual Studio throwing an internal compiler error when compiling zcc_expr.cpp. Even older versions require FMOD and similar stuff, which I don't have on my system.

I reported the crash with lambdas to MS and it's fixed for the next "major" release. Until then you'd need to use the v140 toolset or revert "Use lambdas for more concise code". You don't need FMod to compile (same for FluidSynth and nasm).

[Player701]

You don't need FMod to compile (same for FluidSynth and nasm).

Hmm. I'm not very familiar with CMake. When attempting to configure and generate a VS solution for older verions, it showed an error message: "Configuring incomplete, errors occurred". I assumed it was due to missing dependencies. Unless this bug is fixed before I have the time again (tomorrow evening, maybe), I will check it out later and see if the solution is generated properly.

[drfrag]

I didn't mean it's worth compiling even older versions, you're getting script errors. Check with precompiled binaries in case they can run your mod. It could be there right from the start of ZScript.
You should read the log to know what the error is about (scroll up in CMake GUI).

[Player701]

The script errors were from the scripts in gzdoom.pk3. Granted, there probably weren't that many commits that had them, but I got internal compiler errors on earlier ones. Apparently, there were a few bad revisions that didn't export all required native fields or something like that. Like I said in the first post, the crash happens without any PWADs. I wasn't loading any custom mods to run those tests.

[drfrag]

True, i forgot about that. Have you tried with GZDoom 2.3.x?

[_mental_]

Indeed, why don't you check with stable versions first? The mentioned commit was introduced in 2.4.0 according to GitHub's list of tags. Does 2.3.2 crash? Or 2.2.0? Or even ZDoom 2.8.1?
The idea of bisect relies on having a good commit. Without it a whole endeavor is useless.

[Player701]

I just realized something... the crash is happening only in debug builds. I couldn't get any of the release versions to crash, neither those you mentioned nor the latest devbuild. It is possible this happens because GZDoom references a block of memory that has already been freed, which always triggers a crash in a debug build when it happens (since the pointer is assigned a debug value of 0xdddddddddddddddd, thus making it reference invalid memory). In a release build, however, the chance to get a crash is much lower. I think I got a devbuild crash a while ago, but this time I just can't reproduce it. So I need to compile the respective versions myself (unless there are debug builds available for older versions), which I will do tomorrow unless someone fixes this before then.

[_mental_]

The same psprites member of PredictionPlayerBackup object is freed once from PClass::StaticShutdown() function (it's not nulled there) and the second time from player_t destructor.

[Graf Zahl]

Well, yeah. That cannot work. Leaving a global copy of a variable with destruction semantics lying around unmaintained is just a surefire guarantee of something going wrong.
This field really needs to be cleared out ASAP after having been copied back.

Of course it also needs to be checked if the prediction code can alter this. Normally the psprites member should have std::unique_ptr semantics, preventing the copy of the struct outright.
Let's hope that clearing this pointer after copying back the data is sufficient and the prediction code doesn't muck around with it. If that is the case, all bets are off. Considering the ownership situation here this should really be nulled while predicting, so that this element remains uniquely owned.

[_mental_]

PredictionPlayerBackup is kept between game ticks. Adding psprites = nullptr to P_UnPredictPlayer() won't solve the problem.

[Graf Zahl]

In that case there's two options: Either do not back up the psprites field or add a special check to the destructor so that for the backup it won't get destroyed but only nulled.
I'm not even sure if this was better owned by the PlayerPawn instead, but that entire code shows classic signs of long term development without ever questioning the flawed design: Lots of people messed around with it but nobody ever thought about making it robust.

[_mental_]

Here is the simplest solution.

Moving psprites is a bit problematic as it already exposed to ZScript in PlayerInfo struct.

[Player701]

Oh. I guess I don't have to compile all those old versions now. Thanks a lot for looking into this.

[Nash]

Can be considered fixed: viewtopic.php?p=1094991#p1094991