Chocolate Doom

bzzrak said:

Hi, I wanted to ask this: is autoloading the TNT MAP31 fix an acceptable feature for Choco?

It depends what the feature is that you're proposing. Can you be clearer?

The missing yellow key bug?

IIRC id TeamTNT released a PWAD as a fix for that.

It would make sense for Chocolate Doom to require the same PWAD file that vanilla does.

EDIT: here here.

But I have the pre patched GOG iwad...

Danfun64 said:

But I have the pre patched GOG iwad...

Yes, you use that iwad with "-file tnt31.wad" (which replaces that map with the fixed version.)

You could, but it's pointless. GOG includes the id Anthology version, without the yellow key bug.

There's now a feature request issue raised to autoload tnt31 when necessary

https://github.com/chocolate-doom/chocolate-doom/issues/763

This morning, I was playing a new mod (version 1.05) for vanilla Doom. On E1M5, there's a switch that triggers a floor height change in the room at the south-east of the map. When I press this switch, Chocolate-Doom exists with the following error message:

Sector with more than 22 adjoining sectors. Vanilla will crash here

Source code: https://github.com/chocolate-doom/chocolate-doom/blob/bd7a22c321d9fba5922f4c5c62b403376d88e039/src/doom/p_spec.c#L359

The mod provides a modified vanilla Doom exe with a dehacked patch and Doom+ limits. The Doom+ patch doesn't change this limit (MAX_ADJOINING_SECTORS), so why doesn't it crash with vanilla and why does Chocolate-Doom exits with this error message?

The switch:


In the source code, it's written: "// Emulation of memory (stack) overflow". Is that an accurate emulation? I don't have a map editor to check, but I can't see 22 sectors attached to this sector on the automap.

Usually in DOS, the overflow will start writing into memory past the end of the array, which will have... unpredictable... side effects. I guess it doesn't necessarily crash the game though?

Linguica said:

I guess it doesn't necessarily crash the game though?

In my opinion, Chocolate-Doom is overreacting. Vanilla can run with a corrupted data in the stack without crashing.

It's Entryway who made the change.

axdoom1 said:

In my opinion, Chocolate-Doom is overreacting. Vanilla can run with a corrupted data in the stack without crashing.

I don't think you understand memory corruption and the causes/effects thereof. Especially in memory protected operating systems it's an absolute no-go.

Edward850 said:

I don't think you understand memory corruption and the causes/effects thereof. Especially in memory protected operating systems it's an absolute no-go.

Sure I understand them. If you go past the end of the array, you write over the other data on the stack. If you go too far, out of the memory zone that the OS (Linux or Windows, not DOS) has given to the program, it will kill it.

The fact is, the program should still be able to work if no vital data was overwritten. It will read garbage and maybe have unexpected consequences, but will still work, because in this case it didn't have any fatal consequences. If the "emulation" was accurate, then why was vanilla Doom still able to work for the rest of the time that it took me to complete this level? Vanilla didn't crash, so I explain that by the fact that the overflow was small and wonder if the "emulation" that's found in Chocolate-Doom's code is accurate.

If the emulation is inaccurate, then file a bug report, of course.

axdoom1 said:

Sure I understand them. If you go past the end of the array, you write over the other data on the stack. If you go too far, out of the memory zone that the OS (Linux or Windows, not DOS) has given to the program, it will kill it.

The fact is, the program should still be able to work if no vital data was overwritten. It will read garbage and maybe have unexpected consequences, but will still work, because in this case it didn't have any fatal consequences. If the "emulation" was accurate, then why was vanilla Doom still able to work for the rest of the time that it took me to complete this level? Vanilla didn't crash, so I explain that by the fact that the overflow was small and wonder if the "emulation" that's found in Chocolate-Doom's code is accurate.

That concept only works if Chocolate Doom was actually an emulator (in the strictest sense), though. As the results from said corruption are (from my understanding) undocumented, and the memory locations of said arrays will be vastly different from its DOS counterpart, it will not modify the same memory at all and its incredibly unsafe to just let Chocolate Doom trash memory like that.

The only way that Chocolate Doom can replicate it is if someone can replicate and reconstruct the results in a sandbox, the same way all other overflows are handled in Chocolate and PRBoom+.

Linguica said:

If the emulation is inaccurate, then file a bug report, of course.

Well... I didn't see it as a bug. Since someone (Entryway) made changes to the code and it was committed into the source code tree by Fraggle, I was looking for an explanation by people (Entryway or Fraggle) that know why Chocolate-Doom does this. Else, there may be other intelligent people (I know there is) who are reading this that may be able to explain it.

If they introduced this "emulation" into Chocolate-Doom, I suppose they made research beforehand.

Edward850 said:

the memory locations of said arrays will be vastly different from its DOS counterpart

That's true, Chocolate-Doom has its limits because it's not an emulator. It may be compiled with different compilers and they each do their own thing. GCC will not arrange the code the same way as Watcom C did. Even though Chocolate-Doom can't be perfect, we can make it near perfect (as perfect as possible).

Well... you've found a WAD where the DOS executable has an extremely different experience from Chocolate Doom. (Assuming it wouldn't crash in doom2.exe either, not just the hacked version.) That's certainly a "bug" insofar as Chocolate Doom is intended to emulate the vanilla experience as closely as possible.

Edward850 said:

That concept only works if Chocolate Doom was actually an emulator (in the strictest sense), though. As the results from said corruption are (from my understanding) undocumented, and the memory locations of said arrays will be vastly different from its DOS counterpart, it will not modify the same memory at all and its incredibly unsafe to just let Chocolate Doom trash memory like that.

The only way that Chocolate Doom can replicate it is if someone can replicate and reconstruct the results in a sandbox, the same way all other overflows are handled in Chocolate and PRBoom+.

No, the layout of the stack for this function is knowable. You have to disassemble the target version of the executable in IDA and examine it there. I believed this was what entryway originally did.

Also the way to "emulate" it is, of course, to allow overwrites of the same amount of safety that vanilla would tolerate, by making the array that much larger - not by deliberately performing undefined behaviors. You're thinking about this in a very much wrong sort of way.

Quasar said:

No, the layout of the stack for this function is knowable. You have to disassemble the target version of the executable in IDA and examine it there. I believed this was what entryway originally did.

That's also what I believe Entryway did, but his research is not public, so it's not possible to see why he did what he did.

Linguica said:

bug

Well... You won.

Bug report

axdoom1 said:

In my opinion, Chocolate-Doom is overreacting. Vanilla can run with a corrupted data in the stack without crashing.

Overreacting by exiting with an error you mean? In my opinion allowing the stack to be corrupted is not good or useful behavior.

From my perspective that message has done its job: at the time I added the code it was assumed that no levels caused an overflow of more than 22. Now we've found an example where it's higher. Consider the other possible approaches here:

1. Allow the overflow to be occur. The potential results of this are (a) some other unpredictable behavior occurs which almost certainly does not match Vanilla, and (b) the game crashes anyway, except with no useful information to help diagnose why.
   
2. Ignore the error. This papers over the issue since the game doesn't crash, but we don't get accurate Vanilla behavior. Potentially in the future we end up with demo desyncs which are often tricky to debug.
   

Bombing out with an error instead encourages someone to report the error and then we improve things :)

fraggle said:

Overreacting by exiting with an error you mean? In my opinion allowing the stack to be corrupted is not good or useful behavior.

Allowing the stack to be corrupted is closer to vanilla's behavior than crashing with an error message. An error message was only added in the Linux source code release when an overflow was detected.

Chocolate-Doom could print a warning into stdout. That's what PrBoom seems to do. When the overflow is too big, it just prints a warning saying that it can't emulate the bug and what occurs next is unpredictable.

From PrBoom (p_spec.c):

    for (i=0, h=0 ;i < sec->linecount ; i++)
    {
      check = sec->lines[i];
      other = getNextSector(check,sec);
      
      if (!other)
        continue;
      
      if (other->floorheight > height)
      {
        // e6y
        // Emulation of stack overflow.
        // 20: overflow affects nothing - just a luck;
        // 21: can be emulated;
        // 22..26: overflow affects saved registers - unpredictable behaviour, can crash;
        // 27: overflow affects return address - crash with high probability;
        if (compatibility_level < dosdoom_compatibility && h >= MAX_ADJOINING_SECTORS)
        {
          lprintf(LO_WARN, "P_FindNextHighestFloor: Overflow of heightlist[%d] array is detected.\n", MAX_ADJOINING_SECTORS);
          lprintf(LO_WARN, " Sector %d, line %d, heightlist index %d: ", sec->iSectorID, sec->lines[i]->iLineID, h);

          if (h == MAX_ADJOINING_SECTORS + 1)
            height = other->floorheight;

          if (h <= MAX_ADJOINING_SECTORS + 1)
            lprintf(LO_WARN, "successfully emulated.\n");
          else if (h <= MAX_ADJOINING_SECTORS + 6)
            lprintf(LO_WARN, "cannot be emulated - unpredictable behaviour.\n");
          else
            lprintf(LO_WARN, "cannot be emulated - crash with high probability.\n");
        }
        heightlist[h++] = other->floorheight;
      }
      
      // Check for overflow. Warning.
      if ( compatibility_level >= dosdoom_compatibility && h >= MAX_ADJOINING_SECTORS )
      {
        lprintf( LO_WARN, "Sector with more than 20 adjoining sectors\n" );
        break;
      }
    }

fraggle said:

at the time I added the code it was assumed that no levels caused an overflow of more than 22

Interestingly, in Doom95, the size of the array is 500. Someone may still create a level that breaks this limit. ;-)

Deliberately having a stack corruption error in your program is not acceptable programming practice. The exact behavior should be emulated as closely as possible, and, where it becomes impossible, the program cannot continue to execute, period.

It's not even a choice. Playing fast & loose with the stack will get your program killed. It dumps a core file and you can see right away that you have to fix the code.
(gdb) bt
#0 0x00001aecd9b58a1a in thrkill () at <stdin>:2
#1 0x00001aecd9b5887f in *_libc___stack_smash_handler (
func=0x1aeacc000cca "main", damaged=Variable "damaged" is not available.
)
at /usr/src/lib/libc/sys/stack_protector.c:79
#2 0x00001aeacbf00ca7 in main () from /tmp/a.out

^ That's stupid scanf() example on OpenBSD, but also similar effect is expected on Hardened Gentoo, Alpine Linux, and various others.

Any operating system with randomized memory allocation addresses. Which will include Windows Vista and newer, too.

The big concern here is when creating a demo. You absolutely do not want to save a 1.9 demo when an overflow becomes large enough that it cannot be emulated 100% - that's just not acceptable. It's difficult enough to just get standard 1.9 demos to play without desync. Creating demo files known to be incompatible is a crime against the community. So, 20 and 21 should be emulated.

So, in my opinion, crashing at 22 is a reasonable reaction. Chocolate could arguably instead delete the demo file, pop up a message, and allow the game to continue without recording, but that's an even larger deviation from vanilla.

There's not really much else Chocolate can do here, while also maintaining its mission. Again, allowing a demo file to be saved, after an un-emulatable situation is out of the question.

kb1 said:

There's not really much else Chocolate can do here, while also maintaining its mission. Again, allowing a demo file to be saved, after an un-emulatable situation is out of the question.

The saved game limit setting which also doesn't stop demos, and -longtics, would like a word with you.

Now while I don't disagree without you about keeping the actual limit (well I'm more indifferent but whatever), worrying about how it reflects with demos feels a wee bit too pedantic considering the above.

Random post but I requested a feature over a year ago on cndoom to get vertical mouse sensitivity toggle, and they declined it.

So I did it myself!

Link to binaries for Windows: https://www.dropbox.com/s/1991hb2lspvfa86/cndoom-2.0.3.2-vertmouse.zip?dl=0

To change the vert toggle key: run cnsetup.exe > "Keyboard" > "More Controls..."

Link to source: https://www.dropbox.com/s/pm3efpntjo7hx8c/cndoom-vertmouse-src.7z?dl=0

Note with source that I did the one-way conversion on the .sln to VS2015. That being said, if you have your own cndoom project all the changes to the source code will work with whatever setup you have going :P

Edward850 said:

The saved game limit setting which also doesn't stop demos, and -longtics, would like a word with you.

Now while I don't disagree without you about keeping the actual limit (well I'm more indifferent but whatever), worrying about how it reflects with demos feels a wee bit too pedantic considering the above.

Longtic demos are supposed to be saved as 1.91, if I'm not mistaken, properly invalidating them as true 1.9 demos.

The savegame limit. I don't know (haven't looked) exactly how Chocolate handles the writing of a 1.9 demo when the savegame limit is crossed. At least, that one can be calculated. I think that was a lesser-of-2-evils decision that was not arrived at easily if I recall correctly.

It's not pedantic - many demo archives are very strict on how a demo is created. And, more importantly, demos are just about the only area left where the developers were careful to avoid ambiguity with demo version. Especially a demo marked "1.9". If a demo is marked "1.9", it had just better have a real good chance of playing back in vanilla.

The only "acceptable" reason is that a memory overrun/memory corruption/bad pointer situation has occurred in vanilla, or such a condition was trapped by a source port, in which case the demo should probably be terminated. No reason the game can't keep playing, though.

fraggle said:

Overreacting by exiting with an error you mean? In my opinion allowing the stack to be corrupted is not good or useful behavior.

From my perspective that message has done its job: at the time I added the code it was assumed that no levels caused an overflow of more than 22. Now we've found an example where it's higher. Consider the other possible approaches here:

1. Allow the overflow to be occur. The potential results of this are (a) some other unpredictable behavior occurs which almost certainly does not match Vanilla, and (b) the game crashes anyway, except with no useful information to help diagnose why.
   
2. Ignore the error. This papers over the issue since the game doesn't crash, but we don't get accurate Vanilla behavior. Potentially in the future we end up with demo desyncs which are often tricky to debug.
   

Bombing out with an error instead encourages someone to report the error and then we improve things :)

fraggle... your alive!

this isn't a chocolate-doom-specific issue but it concerns something I got from the chocolate doom wiki. I was playing Chex Quest 2 and realized the dehacked file that makes the Baron stay in place because that's how the encounter was meant to go in Chex Quest, also applies to CQ2, but the Barons at the end of CQ2 are supposed to be mobile. so I figure there needs to be a separate dehacked for CQ2 (I don't know how dehacked files work so I don't know if you could have the current one make an exception if chex2.wad is loaded). I'd make it myself since you'd only have to remove the part that affects the Baron and leave everything else the same, but, I don't know how

Mr.Unsmiley said:

this isn't a chocolate-doom-specific issue but it concerns something I got from the chocolate doom wiki. I was playing Chex Quest 2 and realized the dehacked file that makes the Baron stay in place because that's how the encounter was meant to go in Chex Quest, also applies to CQ2, but the Barons at the end of CQ2 are supposed to be mobile. so I figure there needs to be a separate dehacked for CQ2 (I don't know how dehacked files work so I don't know if you could have the current one make an exception if chex2.wad is loaded). I'd make it myself since you'd only have to remove the part that affects the Baron and leave everything else the same, but, I don't know how

IIRC, in vanilla Chex2, the barons are immobile. It was only in the version released with Chex3 that they move.

xbolt said:

IIRC, in vanilla Chex2, the barons are immobile. It was only in the version released with Chex3 that they move.

oh, I guess you're right. I thought I remembered them moving when I first played it, but that was years ago and probably on a source port that didn't use the correct behavior (zombies dropping ammo, etc). I just looked at zdoom and they don't move there either if you use chex.wad as the iwad, and they don't seem to in dosbox videos on youtube either. well, disregard then