bradharding / doomretro

The classic, refined DOOM source port. For Windows PC.
https://www.doomretro.com
GNU General Public License v3.0
698 stars 88 forks source link

original_states global read access crash #846

Closed devnexen closed 1 month ago

devnexen commented 1 month ago

Reproduced with DOOM 2:

=================================================================
==133759==ERROR: AddressSanitizer: global-buffer-overflow on address 0x559945c42580 at pc 0x5599455e49ed bp 0x7ffe93be45b0 sp 0x7ffe93be45a8
READ of size 4 at 0x559945c42580 thread T0
    #0 0x5599455e49ec in HU_Init /home/dcarlier/Contribs/doomretro/src/hu_stuff.c:217:28
    #1 0x5599455bc013 in D_DoomMainSetup /home/dcarlier/Contribs/doomretro/src/d_main.c:2741:5
    #2 0x5599455b6e88 in D_DoomMain /home/dcarlier/Contribs/doomretro/src/d_main.c:2883:5
    #3 0x5599455c1bc7 in main /home/dcarlier/Contribs/doomretro/src/doomretro.c:212:5
    #4 0x7fabf80a6d67 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #5 0x7fabf80a6e24 in __libc_start_main csu/../csu/libc-start.c:360:3
    #6 0x559945417c40 in _start (/home/dcarlier/Contribs/doomretro/build/doomretro+0x118c40) (BuildId: 5b63dd6572965a8353a17189f4e79621c59f22dd)

0x559945c42580 is located 0 bytes after global variable 'original_states' defined in '/home/dcarlier/Contribs/doomretro/src/states.c:121' (0x559945bf4380) of size 320000
SUMMARY: AddressSanitizer: global-buffer-overflow /home/dcarlier/Contribs/doomretro/src/hu_stuff.c:217:28 in HU_Init

two issues appear :

src/hu_stuff.c lines 213 and 903:

seems the loops are one off, should be :

for (int j = numstates - 1; j >= 0; j--)

src/states.c line 1518:

the game still crashes, seem in there assigning the global to states is not good enough, I did a dynamic (zone) allocation instead and copy the whole array. Then after that the game finally starts. Let me know what you think, hope it helps. Cheers.

bradharding commented 1 month ago

Thanks @devnexen. I'll fix lines 213 and 903 in hu_stuff.c. Not sure what is happening in states.c though, will look into that further. Might be worth using functions in m_array.h in InitStates()...

bradharding commented 1 month ago

@devnexen, could you please confirm these commits resolve this?

devnexen commented 1 month ago

It works perfect, just little comment, name1 member is an array.