DRD Team

Posted: **Tue Nov 29, 2016 20:01**

Steps to reproduce:

load doom2, open the menu then select the 'quit game' menu. When trying to open the menu I get this:

[spoiler]

Code: Select all

=================================================================
==29503==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x608000024fff at pc 0x000000b81de6 bp 0x7ffc945d2b50 sp 0x7ffc945d2b40
READ of size 1 at 0x608000024fff thread T0
    #0 0xb81de5 in OpenGLSWFrameBuffer::OpenGLPal::Update() /home/edward-san/zdoom/qzdoom/src/gl/system/gl_swframebuffer.cpp:2316
    #1 0xb80f45 in OpenGLSWFrameBuffer::OpenGLPal::OpenGLPal(FRemapTable*, OpenGLSWFrameBuffer*) /home/edward-san/zdoom/qzdoom/src/gl/system/gl_swframebuffer.cpp:2238
    #2 0xb82756 in OpenGLSWFrameBuffer::CreatePalette(FRemapTable*) /home/edward-san/zdoom/qzdoom/src/gl/system/gl_swframebuffer.cpp:2401
    #3 0x8c75fd in FRemapTable::GetNative() /home/edward-san/zdoom/qzdoom/src/r_data/r_translate.cpp:337
    #4 0xb8b4db in OpenGLSWFrameBuffer::SetStyle(OpenGLSWFrameBuffer::OpenGLTex*, DrawParms&, unsigned int&, unsigned int&, OpenGLSWFrameBuffer::BufferedTris&) /home/edward-san/zdoom/qzdoom/src/gl/system/gl_swframebuffer.cpp:3422
    #5 0xb84924 in OpenGLSWFrameBuffer::DrawTextureParms(FTexture*, DrawParms&) /home/edward-san/zdoom/qzdoom/src/gl/system/gl_swframebuffer.cpp:2687
    #6 0x1071004 in DCanvas::DrawText(FFont*, int, int, int, char const*, int, ...) /home/edward-san/zdoom/qzdoom/src/v_text.cpp:163
    #7 0x83a811 in DMessageBoxMenu::Drawer() /home/edward-san/zdoom/qzdoom/src/menu/messagebox.cpp:196
    #8 0x8209df in M_Drawer() /home/edward-san/zdoom/qzdoom/src/menu/menu.cpp:725
    #9 0xc37173 in D_Display() /home/edward-san/zdoom/qzdoom/src/d_main.cpp:891
    #10 0xc37d3e in D_DoomLoop() /home/edward-san/zdoom/qzdoom/src/d_main.cpp:1015
    #11 0xc3edc5 in D_DoomMain() /home/edward-san/zdoom/qzdoom/src/d_main.cpp:2645
    #12 0x643daa in main /home/edward-san/zdoom/qzdoom/src/posix/sdl/i_main.cpp:306
    #13 0x7f157237582f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #14 0x635288 in _start (/home/edward-san/zdoom/qzdoom/debug/gcc6_asan/qzdoom+0x635288)

0x608000024fff is located 0 bytes to the right of 95-byte region [0x608000024fa0,0x608000024fff)
allocated by thread T0 here:
    #0 0x7f1574d81f20 in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.3+0xc6f20)
    #1 0xceabfb in M_Malloc_Dbg(unsigned long, char const*, int) /home/edward-san/zdoom/qzdoom/src/m_alloc.cpp:135
    #2 0x8c64d4 in FRemapTable::Alloc(int) /home/edward-san/zdoom/qzdoom/src/r_data/r_translate.cpp:112
    #3 0x8c68ce in FRemapTable::operator=(FRemapTable const&) /home/edward-san/zdoom/qzdoom/src/r_data/r_translate.cpp:169
    #4 0x8c67ad in FRemapTable::FRemapTable(FRemapTable const&) /home/edward-san/zdoom/qzdoom/src/r_data/r_translate.cpp:148
    #5 0x10669da in TArray<FRemapTable, FRemapTable>::Push(FRemapTable const&) /home/edward-san/zdoom/qzdoom/src/tarray.h:243
    #6 0x1059005 in FFont::BuildTranslations(double const*, unsigned char const*, void const*, int, PalEntry const*) /home/edward-san/zdoom/qzdoom/src/v_font.cpp:692
    #7 0x105a4ae in FFont::LoadTranslations() /home/edward-san/zdoom/qzdoom/src/v_font.cpp:870
    #8 0x10578de in FFont::FFont(char const*, char const*, int, int, int, int, int) /home/edward-san/zdoom/qzdoom/src/v_font.cpp:449
    #9 0x1065790 in V_InitFonts() /home/edward-san/zdoom/qzdoom/src/v_font.cpp:2593
    #10 0xff5c06 in R_Init() /home/edward-san/zdoom/qzdoom/src/r_utility.cpp:339
    #11 0xc3e093 in D_DoomMain() /home/edward-san/zdoom/qzdoom/src/d_main.cpp:2461
    #12 0x643daa in main /home/edward-san/zdoom/qzdoom/src/posix/sdl/i_main.cpp:306
    #13 0x7f157237582f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)

SUMMARY: AddressSanitizer: heap-buffer-overflow /home/edward-san/zdoom/qzdoom/src/gl/system/gl_swframebuffer.cpp:2316 in OpenGLSWFrameBuffer::OpenGLPal::Update()
Shadow bytes around the buggy address:
  0x0c107fffc9a0: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 07
  0x0c107fffc9b0: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 07
  0x0c107fffc9c0: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 07
  0x0c107fffc9d0: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 07
  0x0c107fffc9e0: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 07
=>0x0c107fffc9f0: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00[07]
  0x0c107fffca00: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 07
  0x0c107fffca10: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 07
  0x0c107fffca20: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 07
  0x0c107fffca30: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 07
  0x0c107fffca40: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 07
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==29503==ABORTING

[/spoiler]

Posted: **Tue Nov 29, 2016 22:18**

Strange buffer overrun. I've added some additional bounds checking to that code that should hopefully make it stop.

Posted: **Tue Nov 29, 2016 22:38**

It didn't help at all. Same message. Btw, 'i' is 20, so pal[20] is invalid, but also pal[19]...

[edit]maybe this code is the culprit:

Code: Select all

for (i = 0; i < skipat; ++i)
{
	buff[i] = ColorARGB(pal[i].a, pal[i].r, pal[i].g, pal[i].b);
}
for (++i; i < numEntries; ++i)
{
	buff[i] = ColorARGB(pal[i].a, pal[i - 1].r, pal[i - 1].g, pal[i - 1].b);
}

in particular that '++i' in the second 'for'. when that code is reached, 'skipat' is 19, then 'i' is 19 because of the for loop ending, then in the next loop 'i' is increased again and becomes 20.

Posted: **Tue Nov 29, 2016 23:25**

To be honest, I'm not even sure what the code is trying to do. Its ported directly over from the Direct3D 9 target's code. It does the same odd for-looping there.

I thought it was the output buffer that was being overrun, but yeah it might as well be the input one. Another interesting question is, if it buffer overruns in the GL version, does it also do it in the D3D9 version?

Posted: **Wed Nov 30, 2016 10:21**

Who knows. You may add some bound asserts in zdoom D3D9 version and see if it fires or not. If yes it's zdoom's bug.

Posted: **Sun Dec 04, 2016 2:44**

Looking a bit further at this, I still don't understand how there can be a buffer overrun here.

'pal' comes from 'Remap->Palette', which is always allocated to contain 'NumEntries' of elements (by FRemapTable::Alloc).
The 'MIN(Remap->NumEntries, RoundedPaletteSize)' clause in OpenGLSWFrameBuffer::OpenGLPal::Update makes sure that 'skipat' and 'numEntries' can never be larger than Remap->NumEntries.
The skipping of a single color in the middle that you pointed out is odd, but it could still not go out of bounds as the 'i < numEntries' would make it never enter the second loop.

Posted: **Mon Dec 05, 2016 0:10**

Sorry, I should've specified the problem is with this assignment:

Code: Select all

BorderColor = ColorARGB(pal[i].a, pal[i - 1].r, pal[i - 1].g, pal[i - 1].b);

just one line below the two for loops.

Let's assume skipat equals NumEntries and also that NumEntries equals Remap->NumEntries, then the first loop accesses the whole pal[..] array and i becomes equal to NumEntries, then in the second loop, ++i is done and then checked with NumEntries, it fails and then it goes to that piece of code. In that program state, 'i' is 'NumEntries+1' and so, both 'pal' and 'pal[i-1]' are outside the bounds.

I hope it was clear enough.

Posted: **Mon Dec 05, 2016 4:04**

Ah, of course! I must be blind! Thanks Edward-san. And yes, that makes it a buffer overrun in zdoom as well. Makes me wonder if this BorderColor thing is used for anything at all, since it probably had bogus random data in it for decades by now.

Anyhow, committed a fix for this.

Posted: **Mon Dec 05, 2016 10:43**

Then if you can reproduce the issue (by asserts) in zdoom, a new bug report is needed over there.

DRD Team

Buffer overrun in OpenGLSWFrameBuffer

Buffer overrun in OpenGLSWFrameBuffer

Re: Buffer overrun in OpenGLSWFrameBuffer

Re: Buffer overrun in OpenGLSWFrameBuffer

Re: Buffer overrun in OpenGLSWFrameBuffer

Re: Buffer overrun in OpenGLSWFrameBuffer

Re: Buffer overrun in OpenGLSWFrameBuffer

Re: Buffer overrun in OpenGLSWFrameBuffer

Re: Buffer overrun in OpenGLSWFrameBuffer

Re: Buffer overrun in OpenGLSWFrameBuffer