[Fixed] Buffer overflow in sw renderer draw code

Bugs that have been investigated and resolved somehow.

Moderator: GZDoom Developers

Buffer overflow in sw renderer draw code

Postby Edward-san » Sat Jan 30, 2016 7:54 am

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 allExpand view
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
Mathematics is the language with which God has written the universe. (Galilei)
 
Joined: 17 Oct 2009

Re: Buffer overflow in sw renderer draw code

Postby Edward-san » Sun Jan 31, 2016 4:15 am

Here it is what valgrind reports when playing the demo (including the normal output):

Spoiler:
Edward-san
Mathematics is the language with which God has written the universe. (Galilei)
 
Joined: 17 Oct 2009

Re: Buffer overflow in sw renderer draw code

Postby Graf Zahl » Sun Jan 31, 2016 5:53 am

A few comments:

Code: Select allExpand view
==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 allExpand view
==10171==    at 0x6A9CE8: side_t::GetLightLevel(bool, int, bool, int*) const (p_sectors.cpp:1039)


The line in question:

Code: Select allExpand view
   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.
User avatar
Graf Zahl
Lead GZDoom+Raze Developer
Lead GZDoom+Raze Developer
 
Joined: 19 Jul 2003
Location: Germany

Re: Buffer overflow in sw renderer draw code

Postby Edward-san » Sun Jan 31, 2016 4:23 pm

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

Graf Zahl wrote:
Code: Select allExpand view
==10171==    at 0x6A9CE8: side_t::GetLightLevel(bool, int, bool, int*) const (p_sectors.cpp:1039)


The line in question:

Code: Select allExpand view
   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 .
Edward-san
Mathematics is the language with which God has written the universe. (Galilei)
 
Joined: 17 Oct 2009

Re: Buffer overflow in sw renderer draw code

Postby Graf Zahl » Sun Jan 31, 2016 4:36 pm

I don't really understand why it finds this. Isn't this a global variable that's supposed to be initialized to 0?
User avatar
Graf Zahl
Lead GZDoom+Raze Developer
Lead GZDoom+Raze Developer
 
Joined: 19 Jul 2003
Location: Germany

Re: Buffer overflow in sw renderer draw code

Postby Blzut3 » Sun Jan 31, 2016 4:44 pm

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).
Blzut3
Pronounced: B-l-zut
 
 
 
Joined: 24 Nov 2004
Github ID: Blzut3
Operating System: Debian-like Linux (Debian, Ubuntu, Mint, etc) 64-bit
Graphics Processor: ATI/AMD with Vulkan Support

Re: Buffer overflow in sw renderer draw code

Postby Edward-san » Sun Jan 31, 2016 5:01 pm

I think it's because of this:

Code: Select allExpand view
   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
Mathematics is the language with which God has written the universe. (Galilei)
 
Joined: 17 Oct 2009

Re: Buffer overflow in sw renderer draw code

Postby Edward-san » Sun Jan 31, 2016 5:40 pm

Graf Zahl wrote:A few comments:

Code: Select allExpand view
==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 allExpand view
(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 allExpand view
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
Edward-san
Mathematics is the language with which God has written the universe. (Galilei)
 
Joined: 17 Oct 2009

Re: Buffer overflow in sw renderer draw code

Postby Graf Zahl » Sun Jan 31, 2016 6:22 pm

Does this mean that it reads past the end of the array in some situations?
User avatar
Graf Zahl
Lead GZDoom+Raze Developer
Lead GZDoom+Raze Developer
 
Joined: 19 Jul 2003
Location: Germany

Re: Buffer overflow in sw renderer draw code

Postby Edward-san » Sun Jan 31, 2016 6:34 pm

Sounds like it, but surely, 'startForLine[i]' doesn't match the index of the element containing tag=i inside allIDs (goes off by 1).
Edward-san
Mathematics is the language with which God has written the universe. (Galilei)
 
Joined: 17 Oct 2009

Re: Buffer overflow in sw renderer draw code

Postby randi » Sun Jan 31, 2016 10:13 pm

Edward-san wrote:
Code: Select allExpand view
==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?
User avatar
randi
Site Admin
 
Joined: 09 Jul 2003

Re: Buffer overflow in sw renderer draw code

Postby Edward-san » Mon Feb 01, 2016 2:11 am

Nope, I noticed this also before...
Edward-san
Mathematics is the language with which God has written the universe. (Galilei)
 
Joined: 17 Oct 2009

Re: Buffer overflow in sw renderer draw code

Postby Edward-san » Tue Feb 02, 2016 1:45 pm

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.
Edward-san
Mathematics is the language with which God has written the universe. (Galilei)
 
Joined: 17 Oct 2009

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

Postby dpJudas » Wed Jun 08, 2016 9:25 am

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
 
 
 
Joined: 28 May 2016

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

Postby dpJudas » Wed Jun 08, 2016 9:29 am

Oops, probably the wrong thread to type this. But the crash is real. ;)
dpJudas
 
 
 
Joined: 28 May 2016

Next

Return to Closed Bugs

Who is online

Users browsing this forum: No registered users and 0 guests