search

kraflab / dsda-doom

This is a successor of prboom+ with extra tooling for demo recording and playback, with a focus on speedrunning and quality of life.
342 stars 87 forks source link

Fix crash when adding or removing NOBLOCKMAP and NOSECTOR flags #504

Closed XaserAcheron closed 2 months ago

XaserAcheron commented 3 months ago

If MBF21's A_AddFlags or A_RemoveFlags is used to add or set the NOBLOCKMAP or NOSECTOR flags, the game can crash due to a dangling actor pointer. This can occasionally be seen with Legacy of Rust (id1.wad)'s Vassago fireballs when they impact a wal -- it's luck of the draw whether it decides to crash, since it's invalid memory shenanigans, but either way it's buggy. :P

This PR fixes the crash, and adds a new comp_blockmapflags option (not settable via OPTIONS lump since it's strictly a bugfix) that re-enables the buggy behavior for any demos that were recorded before the fix. Attached a quick demo (requires id1.wad) recorded on the release build, just as a demo of the comp functionality.

comp_blockmapflags.lmp.zip

Lemme know if this all looks kosher and I'll make a PR to the MBF21 repo documenting the new comp option (sort of a minor implementation detail in a way, since it's not user-settable, but the docs oughta be accurate ;).

kraflab commented 3 months ago

@fabiangreffrath @rfomin please take a look as well, since we'll need to match behavior in woof

rfomin commented 3 months ago

@fabiangreffrath @rfomin please take a look as well, since we'll need to match behavior in woof

Looks good to me. Thanks, we'll add this.

fabiangreffrath commented 3 months ago

Any chance we get away without another comp flag?

XaserAcheron commented 3 months ago

Maybe -- kraflab would know best here, but the comp flag is only there to make sure that any previously-existing MBF21 demos still sync, even if they ran into the bug. Since this is a crash fix, there's no value at all in letting users set the comp flag, we're just being extra-strict on demo compatibility because that be how dsda do. ;)

That said, I don't even know if there are any such demos out there in the wild -- this goes way out of my area of knowledge though; the dsda crew are the oracles here. If the desync risk is low and everyone's fine with Woof! and company maybe not syncing said demos if they exist, then yoinking the fix without the comp flag will work just fine.

The flag still oughta go in the MBF21 spec regardless, since it does take up a "slot" in the table. The demo playback code will at a minimum need to read the flag and ignore it.

kraflab commented 2 months ago

I think the comp flag is needed in case demos get recorded in the future on versions that don't yet have the fix. This doesn't always crash, so it's entirely possible that future wads come out that have this behavior change but don't crash, which would mean someone may record demos that don't sync in later releases. On the other hand, those demos would be playing the wad with broken behavior unintended by the author, so would they even be important to keep in sync with? 🤔

If we go without a comp flag, we'll have to run a sync check against all existing mbf21 demos.

kraflab commented 2 months ago

Putting this through regression testing right now. If there are no issues with existing demos we'll go without the comp flag.

kraflab commented 2 months ago

No desyncs, just taking this then without the flag: https://github.com/kraflab/dsda-doom/commit/c892178239f8729dcb0748844930e381d4bdd2db @rfomin @fabiangreffrath