mbbsemu / MBBSEmu

The MajorBBS Emulation Project is an Open Source, Cross-Platform emulator for easily running The MajorBBS & Worldgroup Modules
https://www.mbbsemu.com
MIT License
126 stars 14 forks source link

[WCCMMUD] Majormud - `help exp` command can cause the emulator to hit 100% CPU and appear to be hung #613

Closed fletcherm closed 6 months ago

fletcherm commented 6 months ago

Module Information

Describe the bug

Running the help exp command can occasionally cause the emulator to appear like it is hung. It pegs the CPU at 100% and is no longer usable until restarted.

To Reproduce

This was first found by bad luck via the very occasional use of the help exp command. But to easily reproduce it:

  1. Enter the game with any created character.
  2. Using your client of choice, create a macro for help exp^M.
  3. Hold down the macro key or otherwise trigger it to run as fast as possible.
  4. Within ~10 seconds one of the command will cause the board to hang.

Expected behavior

The board should never hang from this command.

Additional info

Software Information:

enusbaum commented 6 months ago

I was able to replicate this locally, looking into it!

enusbaum commented 6 months ago

The issue appeared to be in this code here:

image

This routine loads commands to parse into a string and uses SKPWRD and SKPWHT to look through the string using SAMETO to compare it to the command the user entered to get the help topic to load.

This string gets loaded to an allocated memory address and in very specific scenarios, if the end of the string ended at 0xFFFF, it would cause SKPWRD to result in the above logic to loop infinitely because the terminating \0 was at 0xFFFF which the internal loop would never reach because the loop evaluated < ushort.MaxValue when it should evaluate <= ushort.MaxValue

enusbaum commented 6 months ago

Offending Line: https://github.com/mbbsemu/MBBSEmu/blob/a2eedf897932d84bd399b6dffdff98b392896bac/MBBSEmu/HostProcess/ExportedModules/Majorbbs.cs#L7215

enusbaum commented 6 months ago

I'm writing Unit Tests for both SKPWHT and SKPWRD as well as writing a Helper Method that will help test for this scenario where if I want to allocate the string HELLO\0, it will allocate it at 0xFFF9 allowing us to test for this scenario where the final \0 is at offset 0xFFFF as there might be some other methods that use similar logic.