Occasional crash when exiting levels

[MrAlaux]

As reported by a few users on the Doomworld thread; I seemingly encountered it myself, too. Very infrequent, I'd say.

It seems to happen upon the first exit of the session (i.e. since the game was launched).

Probably related to the Alternative Intermission Background.

[ceski-1]

Here's an ASAN build you can share with users: nugget-doom-debug.zip (Windows 10/11 x64 only)
It's based on this commit: 331c2b3

The game should be launched with _nugget-doom-debug.bat which uses this command line:

nugget-doom.com -skill 4 -warp 1 -verbose > _log.txt 2>&1

The parameters can be changed as needed but -verbose > _log.txt 2>&1 should be left alone. If the game crashes, _log.txt should be copied somewhere else since it's overridden each launch.

[MrAlaux]

Here's an ASAN build you can share with users

Many thanks!
Do you mind if I link users from DW directly to this comment of yours? It would appear that you're keeping a low profile for some reason.

[ceski-1]

Do you mind if I link users from DW directly to this comment of yours? It would appear that you're keeping a low profile for some reason.

Feel free, I don't mind. My attention has been focused elsewhere, so I can't dive in myself just yet.

[MrAlaux]

We got something:

Crash log

Nugget Doom 3.1.0 (built on Jun 18 2024)

M_LoadDefaults: Load system defaults.
 default file: D:\Source Ports\idTech1\nugget-doom-debug\nugget-doom.cfg

IWAD found: D:\Steam\steamapps\common\doom 2\base\doom2.wad
DOOM II version

Savegame directory: D:\Source Ports\idTech1\nugget-doom-debug\savegames\doom2.wad
Screenshot directory: .

W_Init: Init WADfiles.
 adding D:\Steam\steamapps\common\doom 2\base\doom2.wad
 adding D:\Source Ports\idTech1\nugget-doom-debug\autoload\all-all\woofhud.lmp
 adding D:\Source Ports\idTech1\nugget-doom-debug\autoload\doom-all\brghtmps.lmp

M_Init: Init miscellaneous info.
R_Init: Init DOOM refresh daemon - [                          ]........................
P_Init: Init Playloop state.
I_Init: Setting up machine state.
I_InitController: Initialize game controller.
I_InitSound:
 Using 'OpenAL Soft on Speakers (Realtek(R) Audio)' @ 48000 Hz.
 Using 'Linear' resampler.
 Precaching all sound effects... done.
MIDI Init: Using 'CoolSoft MIDIMapper'.
NET_Init: Init network subsystem.
D_CheckNetGame: Checking network game status.
 startskill 3  deathmatch: 0  startmap: 1  startepisode: 1
 player 1 of 1 (1 nodes)
S_Init: Setting up sound.
HU_Init: Setting up heads up display.
ST_Init: Init status bar.
VX_Init: Voxels not found.

SDL render driver: direct3d
ResetResolution: 320x200
S_ChangeMusic: D_RUNNIN (doom2.wad)
P_SetupLevel: MAP01 (doom2.wad), Skill 4, Doom, Doom 1.9
S_ChangeMusic: D_IN_CIT (doom2.wad)
P_SetupLevel: MAP09 (doom2.wad), Skill 4, Doom, Doom 1.9
G_DoLoadGame: Slot 00, Time (1:09) 0:00.69
=================================================================
==3756==ERROR: AddressSanitizer: access-violation on unknown address 0x000000000000 (pc 0x7ffb9d09ca07 bp 0x000000000000 sp 0x00b54dcff160 T0)
==3756==The signal is caused by a READ memory access.
==3756==Hint: address points to the zero page.
    #0 0x7ffb9d09ca06 in add_string_to_line C:\msvc\Nugget-Doom\src\hu_lib.c:189
    #1 0x7ffb9d09ac16 in HUlib_add_strings_to_cur_line C:\msvc\Nugget-Doom\src\hu_lib.c:215
    #2 0x7ffb9d09a96e in HUlib_add_string_to_cur_line C:\msvc\Nugget-Doom\src\hu_lib.c:222
    #3 0x7ffb9d0aab4e in HU_widget_build_sttime C:\msvc\Nugget-Doom\src\hu_stuff.c:1487
    #4 0x7ffb9d0a16b8 in HU_widget_rebuild_sttime C:\msvc\Nugget-Doom\src\hu_stuff.c:1492
    #5 0x7ffb9d0929dd in G_DoCompleted C:\msvc\Nugget-Doom\src\g_game.c:1756
    #6 0x7ffb9d086e3e in G_Ticker C:\msvc\Nugget-Doom\src\g_game.c:3106
    #7 0x7ffb9d07a231 in RunTic C:\msvc\Nugget-Doom\src\d_net.c:91
    #8 0x7ffb9d06d6e3 in TryRunTics C:\msvc\Nugget-Doom\src\d_loop.c:878
    #9 0x7ffb9d079be9 in D_DoomMain C:\msvc\Nugget-Doom\src\d_main.c:3035
    #10 0x7ffb9d0b877e in NuggetDoom_Main C:\msvc\Nugget-Doom\src\i_main.c:70
    #11 0x7ff72d66283c  (D:\Source Ports\idTech1\nugget-doom-debug\nugget-doom.com+0x14000283c)
    #12 0x7ff72d66a12b  (D:\Source Ports\idTech1\nugget-doom-debug\nugget-doom.com+0x14000a12b)
    #13 0x7ff72d66a1d2  (D:\Source Ports\idTech1\nugget-doom-debug\nugget-doom.com+0x14000a1d2)
    #14 0x7ff72d663e78  (D:\Source Ports\idTech1\nugget-doom-debug\nugget-doom.com+0x140003e78)
    #15 0x7ff72d663d61  (D:\Source Ports\idTech1\nugget-doom-debug\nugget-doom.com+0x140003d61)
    #16 0x7ff72d663c1d  (D:\Source Ports\idTech1\nugget-doom-debug\nugget-doom.com+0x140003c1d)
    #17 0x7ff72d663f0d  (D:\Source Ports\idTech1\nugget-doom-debug\nugget-doom.com+0x140003f0d)
    #18 0x7ffc0ba07343 in BaseThreadInitThunk+0x13 (C:\WINDOWS\System32\KERNEL32.DLL+0x180017343)
    #19 0x7ffc0bfbcc90 in RtlUserThreadStart+0x20 (C:\WINDOWS\SYSTEM32\ntdll.dll+0x18004cc90)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: access-violation C:\msvc\Nugget-Doom\src\hu_lib.c:189 in add_string_to_line
==3756==ABORTING

[MrAlaux]

@fabiangreffrath, I hope you don't mind me bringing the discussion here (which I should have done to begin with).

How exactly could fabiangreffrath@80e7fa3 be fixing it? If I'm not mistaken, that merely initializes hud_timestr[], which I guess could get rid of potential garbage that'd allow a buffer overflow (if that's possible with the HUlib code, which I haven't checked), but the description of the error doesn't sound like it. Could it be that w_sttime is the one being used uninitialized?

[fabiangreffrath]

HU_widget_build_sttime() is called unconditionally when preparing the intermission screen in G_DoCompleted(). If the former function has never been called before, because the level time widget was disabled, then the hud_timestr[] string would still be uninitialized at this point. However, in the last line of that function, the string is loaded into the w_sttime widget by HUlib_add_string_to_cur_line(), which ultimately calls add_string_to_line(), which in turn counts the string width in pixels. If the string still consists of garbage at this point, an overflow will occur.

NB: A widget line is allowed to contain HU_MAXLINELENGTH chars, and this is checked in add_char_to_line(). However, the lines are never explicitly terminated.

[MrAlaux]

This leaves me wondering why Woof wasn't affected; I'm pretty sure that it should've been, but I don't recall any reports.

[fabiangreffrath]

However, the lines are never explicitly terminated.

I was wrong. They are terminated, in the same function.

This leaves me wondering why Woof wasn't affected; I'm pretty sure that it should've been, but I don't recall any reports.

Good question. Is it possible that people were playing with ASAN builds?

[fabiangreffrath]

Doesn't appear to be an overflow, though. This patch reliably crashes the game:

--- a/src/hu_stuff.c
+++ b/src/hu_stuff.c
@@ -1445,6 +1445,7 @@ static void HU_widget_build_sttime(void)
   char hud_timestr[HU_MAXLINELENGTH/2];
   int offset = 0;
   extern int time_scale;
+  memset(hud_timestr, HU_FONTEND + 6 + 1, sizeof(hud_timestr));
 
   if ((hud_level_time & HUD_WIDGET_HUD     && !automapactive) ||
       (hud_level_time & HUD_WIDGET_AUTOMAP &&  automapactive))

The issue is that you add the width of certain glyphs to the string width, but these glyphs haven't been added to the font before. Of course, these glyphs are never added to a regular hud_timestr, but if it contains garbage, chances are it also contains these. This also explains why Woof is not affected.

Nugget-Doom/src/hu_lib.c

Lines 188 to 189 in 331c2b3

	else if (c >= HU_FONTSTART && c <= HU_FONTEND + 6 + 3) // [Nugget] Stats icons
	w += SHORT(p[c - HU_FONTSTART]->width);

[fabiangreffrath]

The issue is that you add the width of certain glyphs to the string width, but these glyphs haven't been added to the font before.

How about this:

--- a/src/hu_stuff.c
+++ b/src/hu_stuff.c
@@ -440,6 +440,7 @@ void HU_Init(void)
   for (i = HU_FONTSIZE + 6, j = 0; j < 3; i++, j++)
   {
     static const char *names[] = { "HUDKILLS", "HUDITEMS", "HUDSCRTS" };
+    static const char fallback[] = { 'K', 'I', 'S' };
     const char *icon = names[j];
 
     if (W_CheckNumForName(icon) != -1)
@@ -447,7 +448,11 @@ void HU_Init(void)
       sml_font.patches[i] =
       big_font.patches[i] = (patch_t *) W_CacheLumpName(icon, PU_STATIC);
     }
-    else { sml_font.patches[i] = big_font.patches[i] = NULL; }
+    else
+    {
+      sml_font.patches[i] = sml_font.patches[fallback[j]-HU_FONTSTART];
+      big_font.patches[i] = big_font.patches[fallback[j]-HU_FONTSTART];
+    }
   }
 
   // [FG] calculate font height once right here

[MrAlaux]

How about this

Is it really necessary if we zero-initialize hud_timestr[]?

[fabiangreffrath]

Is it really necessary if we zero-initialize hud_timestr[]?

Nope, just future-proof.

[MrAlaux]

Nope, just future-proof.

Fair enough. I'll apply these fixes later. Thanks for your research!

FWIW, and a bit of a note-to-self: when I said that the description of the error didn't sound like a buffer overflow, I was talking about how the crash log mentioned an attempt to access address zero. The stats icons being initialized to NULL if lumps weren't found explains that.