Buffer-overflow in font.cpp via widget draw

[pjbroad]

I experienced a buffer-overflow crash while switching to the "Server" tab of options. I think it was the first time viewing the tab. I've so far been unable to trace the problem so posting here in case someone else can find it. I'm working on a patch that does not touch most of this code but has inserted a function into elwindows.c so those line numbers are off compared to git latest but its the obvious widget calling code. My lang string is set to "en".

Here's the output from the address sanitizer:

READ of size 1 at 0x55cf41b9564a thread T0
    #0 0x55cf41643aea in eternal_lands::Font::line_width(unsigned char const*, unsigned long, float) const /home/paul/el/elc/font.cpp:450
    #1 0x55cf4165b00e in eternal_lands::FontManager::line_width(font_cat, unsigned char const*, unsigned long, float) /home/paul/el/elc/font.h:1289
    #2 0x55cf4165b00e in get_buf_width_zoom /home/paul/el/elc/font.cpp:1877
    #3 0x55cf41558579 in pword_field_draw /home/paul/el/elc/widgets.c:3930
    #4 0x55cf413ee6ea in draw_window /home/paul/el/elc/elwindows.c:1678
    #5 0x55cf413ee85d in display_window /home/paul/el/elc/elwindows.c:1845
    #6 0x55cf413ee98a in display_windows /home/paul/el/elc/elwindows.c:544
    #7 0x55cf413cec08 in draw_scene /home/paul/el/elc/draw_scene.c:135
    #8 0x55cf4145d294 in start_rendering /home/paul/el/elc/main.c:235
    #9 0x55cf4145e35c in main /home/paul/el/elc/main.c:557
    #10 0x7f7c23c75cb1 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x28cb1)
    #11 0x55cf4126f86d in _start (/home/paul/el/builds/dev_build/el.linux.bin+0x10986d)

0x55cf41b9564a is located 54 bytes to the left of global variable 'isometric' defined in '/home/paul/el/elc/elconfig.c:298:5' (0x55cf41b95680) of size 4
0x55cf41b9564a is located 0 bytes to the right of global variable 'lang' defined in '/home/paul/el/elc/elconfig.c:302:6' (0x55cf41b95640) of size 10
SUMMARY: AddressSanitizer: global-buffer-overflow /home/paul/el/elc/font.cpp:450 in eternal_lands::Font::line_width(unsigned char const*, unsigned long, float) const
Shadow bytes around the buggy address:
  0x0aba6836aa70: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x0aba6836aa80: 04 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9
  0x0aba6836aa90: 04 f9 f9 f9 f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9
  0x0aba6836aaa0: 04 f9 f9 f9 f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9
  0x0aba6836aab0: 04 f9 f9 f9 f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9
=>0x0aba6836aac0: 04 f9 f9 f9 f9 f9 f9 f9 00[02]f9 f9 f9 f9 f9 f9
  0x0aba6836aad0: 04 f9 f9 f9 f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9
  0x0aba6836aae0: 04 f9 f9 f9 f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9
  0x0aba6836aaf0: 04 f9 f9 f9 f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9
  0x0aba6836ab00: 04 f9 f9 f9 f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9
  0x0aba6836ab10: 04 f9 f9 f9 f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9
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
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  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
  Shadow gap:              cc
==33314==ABORTING

[gvissers]

Will take a look, probably not today though. I haven't been able to reproduce it yet, from your post I think that this was a one-time crash for you as well, am I correct? Or are you able to crash it reliably? Did it crash immediately upon opening the the tab, or were you moving the mouse cursor over an input field or something?

EDIT: also: did you change anything in the global variables in elconfig.c? I'm thinking about the "54 bytes to the left of global variable 'isometric'" and "0 bytes to the right of global variable 'lang'" part. Even when aligning ints to 8 bytes the math there doesn't completely add up. Of course the compiler can do what it wants, I just wonder.

[pjbroad]

Thanks. I cannot reproduce it. I'm pretty sure the crash happened while scrolling the page. My changes have not altered elconfig.c other than move a #def to another module. The extent of my investigation was looking at the code around the cursor handling but I could not see anything obvious.

[gvissers]

I have pushed a few changes that will fix a few possible buffer overruns in elconfig, but not for the language option: the declared buffer size was 10 bytes, an the size given in add_var() was 8 bytes, so even if change_string() would put a terminator after the 8'th byte, it would be stored inside the buffer. The handling of string buffers in the options is now more in line with the rest of the client though, with the terminator being included in the character count.

Still looking what might have gone wrong...

[gvissers]

Found it. While scrolling you probably click in the small space between the border and the the first character in the field by accident.

[pjbroad]

Nicely done! Thanks very much. Closing as fixed by 081c66b