Read past the buffer end in FInternalLightAssociation code

[Edward-san]

Valgrind detected a problem when gzdoom is run with lights.pk3:

Code: Select all

==18732== Conditional jump or move depends on uninitialised value(s)
==18732==    at 0x82B6E7: FInternalLightAssociation::FInternalLightAssociation(FLightAssociation*) (gl_dynlight.cpp:975)
==18732==    by 0x82B7D0: gl_InitializeActorLights() (gl_dynlight.cpp:1022)
==18732==    by 0x82C2E0: gl_ParseDefs() (gl_dynlight.cpp:1344)
==18732==    by 0x8207D6: FGLInterface::Init() (gl_scene.cpp:1137)
==18732==    by 0x6D1B8B: R_Init() (r_utility.cpp:540)
==18732==    by 0x5B513A: D_DoomMain() (d_main.cpp:2251)
==18732==    by 0x5749FA: main (i_main.cpp:349)

with gdb I got this info:

Code: Select all

Program received signal SIGTRAP, Trace/breakpoint trap.
0x000000000082b6e7 in FInternalLightAssociation::FInternalLightAssociation (
    this=0x15d0ef10, asso=0x15d026e0)
    at /home/edward-san/zdoom/gzdoom/old/src/gl/dynlights/gl_dynlight.cpp:975
975		if (strlen(asso->FrameName())==5 || asso->FrameName()[5]=='0')
(gdb) p asso->FrameName()
$2 = 0x15d026e0 "BAR1"

So it seems that the code doesn't know about the existence of frame names shorter than 5 characters...

And this happens since at least gzdoom 323 ( yeah, I noticed that in zandronum, first )

[Graf Zahl]

That problem most certainly comes from the original ZDoomGL code but was never discovered before.
Well, things happen, I've never seen it cause a problem so far.

[Graf Zahl]

Had a look, this is completely harmless because it only checks a static 8 character array.
I just changed the initialization from an sprintf to strncpy which zeros the remaining elements - no idea why this used the more inefficient function

[Edward-san]

Just a curiosity: is it there no way that frameName could contain 8 non-null characters? I wonder if the m_FrameName should be char[9] instead of char[8]...

[Gez]

Just a curiosity: is it there no way that frameName could contain 8 non-null characters? I wonder if the m_FrameName should be char[9] instead of char[8]...

Sprite name: 4 characters
Frame letter: 1 character
Frame name = sprite name + frame letter = 5 characters

It's not a lump name. When you have something like BOS2A2C8, it's eight non-null characters, but it's not a frame name. It's a graphic lump name. Frame name here are BOS2A and BOS2C.