Buffer overflow in sw renderer draw code

Bugs that have been investigated and resolved somehow.

Moderator: GZDoom Developers

Forum rules
Please don't bump threads here if you have a problem - it will often be forgotten about if you do. Instead, make a new thread here.
Edward-san
Posts: 1774
Joined: Sat Oct 17, 2009 9:40 am

Buffer overflow in sw renderer draw code

Post by Edward-san »

If zdoom is compiled with GCC 5, "C(XX)FLAGS=-g3 -fsanitize=address -fno-omit-frame-pointer", you can see a particularly noisy buffer overflow, which can be reproduced this way:

1) load zdoom with the attached demo, like this:

Code: Select all

zdoom -iwad doom.wad -file kdizd_12.pk3 -playdemo kdizd_crash_bad.lmp -nosound
this demo will show a way to reproduce the bug, by warping to the little monitor room at the entryway and rotating around in a way the monitor should show up, then the bug appears.

The message I got from the address sanitizer is this:
Spoiler:
From what I see in the debugger, in r_draw.cpp line 1677, somehow 'source' is accessed past the its end. I wonder if there could be a way to reproduce this without using the sanitizer...
You do not have the required permissions to view the files attached to this post.
Edward-san
Posts: 1774
Joined: Sat Oct 17, 2009 9:40 am

Re: Buffer overflow in sw renderer draw code

Post by Edward-san »

Here it is what valgrind reports when playing the demo (including the normal output):
Spoiler:
User avatar
Graf Zahl
Lead GZDoom+Raze Developer
Lead GZDoom+Raze Developer
Posts: 49183
Joined: Sat Jul 19, 2003 10:19 am
Location: Germany

Re: Buffer overflow in sw renderer draw code

Post by Graf Zahl »

A few comments:

Code: Select all

==10171== Conditional jump or move depends on uninitialised value(s)
==10171==    at 0x6C9AFC: FTagManager::LineHasID(int, int) const (p_tags.cpp:255)
==10171==    by 0x6AEA90: P_FinishLoadingLineDef(line_t*, int) (p_setup.cpp:2052)
I cannot confirm that there's ever uninitialized values in the tag list. Even adding asserts left and right in that code never triggers them.

Code: Select all

==10171==    at 0x6A9CE8: side_t::GetLightLevel(bool, int, bool, int*) const (p_sectors.cpp:1039)
The line in question:

Code: Select all

	if (!foggy || level.flags3 & LEVEL3_FORCEFAKECONTRAST) // Don't do relative lighting in foggy sectors
So...
- foggy is a function parameter so it cannot possibly be uninitialized, especially since in all cases the same global variable gets passed in.
- level.flags3 is a global variable that gets set for each new level.

So I have no idea how this can access uninitialized memory...
And since this error constitutes the bulk of the entire report I have no idea what to make of it.
Edward-san
Posts: 1774
Joined: Sat Oct 17, 2009 9:40 am

Re: Buffer overflow in sw renderer draw code

Post by Edward-san »

I'll see what the debugger tells, but in the meantime:
Graf Zahl wrote:

Code: Select all

==10171==    at 0x6A9CE8: side_t::GetLightLevel(bool, int, bool, int*) const (p_sectors.cpp:1039)
The line in question:

Code: Select all

	if (!foggy || level.flags3 & LEVEL3_FORCEFAKECONTRAST) // Don't do relative lighting in foggy sectors
So...
- foggy is a function parameter so it cannot possibly be uninitialized, especially since in all cases the same global variable gets passed in.
- level.flags3 is a global variable that gets set for each new level.

So I have no idea how this can access uninitialized memory...
And since this error constitutes the bulk of the entire report I have no idea what to make of it.
Actually, the issue was with level.flags3, which was not initialized in level_info_t::Reset. See https://github.com/rheit/zdoom/pull/527 .
User avatar
Graf Zahl
Lead GZDoom+Raze Developer
Lead GZDoom+Raze Developer
Posts: 49183
Joined: Sat Jul 19, 2003 10:19 am
Location: Germany

Re: Buffer overflow in sw renderer draw code

Post by Graf Zahl »

I don't really understand why it finds this. Isn't this a global variable that's supposed to be initialized to 0?
Blzut3
 
 
Posts: 3178
Joined: Wed Nov 24, 2004 12:59 pm
Graphics Processor: ATI/AMD with Vulkan/Metal Support

Re: Buffer overflow in sw renderer draw code

Post by Blzut3 »

Yes but the level_info_t that gets copied into FLevelLocals isn't. Valgrind transfers uninitialized state on assignments (might be down to the bit level, but not sure on that one).
Edward-san
Posts: 1774
Joined: Sat Oct 17, 2009 9:40 am

Re: Buffer overflow in sw renderer draw code

Post by Edward-san »

I think it's because of this:

Code: Select all

	level.flags3 |= info->flags3;
in function G_InitLevelLocals in g_level.cpp line 1355. level is global but then it's overridden by the info received through FindLevelInfo call in line 1303.
Edward-san
Posts: 1774
Joined: Sat Oct 17, 2009 9:40 am

Re: Buffer overflow in sw renderer draw code

Post by Edward-san »

Graf Zahl wrote:A few comments:

Code: Select all

==10171== Conditional jump or move depends on uninitialised value(s)
==10171==    at 0x6C9AFC: FTagManager::LineHasID(int, int) const (p_tags.cpp:255)
==10171==    by 0x6AEA90: P_FinishLoadingLineDef(line_t*, int) (p_setup.cpp:2052)
I cannot confirm that there's ever uninitialized values in the tag list. Even adding asserts left and right in that code never triggers them.
I run the debugger + valgrind (i.e. vgdb), then I noticed that in p_tags.cpp line 255:

Code: Select all

(gdb) p i
$10 = 17811
(gdb) p ndx
$11 = 1279
(gdb) p allIDs[ndx]
$12 = (FTagItem &) @0xf121694: {target = 0, tag = 975800, nexttag = 0}
(gdb) p allIDs[ndx-1]
$13 = (FTagItem &) @0xf121688: {target = 17811, tag = 0, nexttag = -1}
and allIDs[ndx-1] matches '{ line, tag, -1 }' seen in line 145 when I make a conditioned breakpoint (allIDs.Size() == 1278). I hope this helps.

Edit: added the output with the breakpoint reached:

Code: Select all

Breakpoint 1, FTagManager::AddLineID (this=0xcda160 <tagManager>, line=17811, 
    tag=0) at /home/edward-san/zdoom/trunk/src/p_tags.cpp:146
146		allIDs.Push(it);
(gdb) p allIDs
$1 = {Array = 0xf11daa0, Most = 1369, Count = 1278}
(gdb) p line
$2 = 17811
(gdb) p tag
$3 = 0
User avatar
Graf Zahl
Lead GZDoom+Raze Developer
Lead GZDoom+Raze Developer
Posts: 49183
Joined: Sat Jul 19, 2003 10:19 am
Location: Germany

Re: Buffer overflow in sw renderer draw code

Post by Graf Zahl »

Does this mean that it reads past the end of the array in some situations?
Edward-san
Posts: 1774
Joined: Sat Oct 17, 2009 9:40 am

Re: Buffer overflow in sw renderer draw code

Post by Edward-san »

Sounds like it, but surely, 'startForLine' doesn't match the index of the element containing tag=i inside allIDs (goes off by 1).
User avatar
randi
Site Admin
Posts: 7749
Joined: Wed Jul 09, 2003 10:30 pm

Re: Buffer overflow in sw renderer draw code

Post by randi »

Edward-san wrote:

Code: Select all

==18097==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x62b000076e00 at pc 0x000000904c57 bp 0x7fff79bfcda0 sp 0x7fff79bfcd98
READ of size 1 at 0x62b000076e00 thread T0
    #0 0x904c56 in vlinec1 /home/edward-san/zdoom/trunk/src/r_draw.cpp:1677
    #1 0x932a7c in wallscan(int, int, short*, short*, int*, int*, int, unsigned char const* (*)(FTexture*, int)) /home/edward-san/zdoom/trunk/src/r_segs.cpp:1209
Could this be from the right edge in/ex-clusivity mixing that was just fixed?
Edward-san
Posts: 1774
Joined: Sat Oct 17, 2009 9:40 am

Re: Buffer overflow in sw renderer draw code

Post by Edward-san »

Nope, I noticed this also before...
Edward-san
Posts: 1774
Joined: Sat Oct 17, 2009 9:40 am

Re: Buffer overflow in sw renderer draw code

Post by Edward-san »

I admit the tag error seems weird, considering that when I run the normal zdoom program in debug mode, the value is not uninitialized and matches the data in the function FTagManager::AddLineID, so ignore that for now.

Now the only thing left is the buffer overflow thing, which seems to be reported by both the address sanitizer and valgrind. Just to clarify: I noticed this in the same period of time as the right edge error.
dpJudas
 
 
Posts: 3134
Joined: Sat May 28, 2016 1:01 pm

Re: [>=g240ca2af] Buffer overflow in wallscan code (again)

Post by dpJudas »

I think I may have managed to trigger this in my truecolor branch. At least I detected a buffer underflow as illustrated by the following debugger output:

Image

As you can see, dc_texturefrac somehow manages to have a negative value, which in turn causes it do a buffer overrun. Depending on where the column is in the texture it can cause zdoom to crash.
dpJudas
 
 
Posts: 3134
Joined: Sat May 28, 2016 1:01 pm

Re: [>=g240ca2af] Buffer overflow in wallscan code (again)

Post by dpJudas »

Oops, probably the wrong thread to type this. But the crash is real. ;)

Return to “Closed Bugs [GZDoom]”