[models] possible heap buffer overflow

[chriscamacho]

Issue description

=================================================================
==15869==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61d000044a7e at pc 0x7fd38310ae1d bp 0x7ffe25c5d140 sp 0x7ffe25c5c8e8
READ of size 1985 at 0x61d000044a7e thread T0
#0 0x7fd38310ae1c (/usr/lib/libasan.so.5+0x66e1c)
#1 0x561e216f2c48 in my_strndup (/home/chris/development/raylib/hexRPG/hexRPG+0x5dc48)
#2 0x561e216f1eb2 in tinyobj_parse_obj (/home/chris/development/raylib/hexRPG/hexRPG+0x5ceb2)
#3 0x561e216fb36f in LoadOBJ (/home/chris/development/raylib/hexRPG/hexRPG+0x6636f)
#4 0x561e216fb063 in LoadModel (/home/chris/development/raylib/hexRPG/hexRPG+0x66063)
#5 0x561e216a12df in main src/main.c:164
#6 0x7fd382c5bdea in __libc_start_main ../csu/libc-start.c:308
#7 0x561e2169df19 in _start (/home/chris/development/raylib/hexRPG/hexRPG+0x8f19)

0x61d000044a7e is located 0 bytes to the right of 2046-byte region [0x61d000044280,0x61d000044a7e)
allocated by thread T0 here:
#0 0x7fd3831b0458 in __interceptor_malloc (/usr/lib/libasan.so.5+0x10c458)
#1 0x561e216fb322 in LoadOBJ (/home/chris/development/raylib/hexRPG/hexRPG+0x66322)

SUMMARY: AddressSanitizer: heap-buffer-overflow (/usr/lib/libasan.so.5+0x66e1c)
Shadow bytes around the buggy address:
0x0c3a800008f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0c3a80000900: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0c3a80000910: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0c3a80000920: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0c3a80000930: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c3a80000940: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00[06]
0x0c3a80000950: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c3a80000960: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c3a80000970: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c3a80000980: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c3a80000990: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
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
==15869==ABORTING

Environment

linux 64 bit, statically linking to raylib, running address sanitize (was wanting to check my code not yours :p )

Code Example

any / first occurrence of LoadModel

I note that this issue has been reported / fixed before ?

[raysan5]

@chriscamacho I'll take a look... any idea? did you check LoadOBJ()?

[chriscamacho]

I'm just figuring out how to recompile raylib with debug symbols, I'll let you know if I get anywhere with it...

[chriscamacho]

READ of size 1985 at 0x61d000044a7e thread T0
#0 0x7fa91dea6e1c (/usr/lib/libasan.so.5+0x66e1c)
#1 0x5579359f7879 in my_strndup external/tinyobj_loader_c.h:482
#2 0x5579359f5e9d in tinyobj_parse_obj external/tinyobj_loader_c.h:1314
#3 0x557935a068aa in LoadOBJ /home/chris/development/raylib/raylib/src/models.c:2784
#4 0x557935a06460 in LoadModel /home/chris/development/raylib/raylib/src/models.c:638
#5 0x55793597929f in main src/main.c:164
#6 0x7fa91d9f7dea in __libc_start_main ../csu/libc-start.c:308
#7 0x557935975ed9 in _start (/home/chris/development/raylib/hexRPG/hexRPG+0x8ed9)

0x61d000044a7e is located 0 bytes to the right of 2046-byte region [0x61d000044280,0x61d000044a7e)
allocated by thread T0 here:
#0 0x7fa91df4c458 in __interceptor_malloc (/usr/lib/libasan.so.5+0x10c458)
#1 0x557935a06818 in LoadOBJ /home/chris/development/raylib/raylib/src/models.c:2774
#2 0x557935a06460 in LoadModel /home/chris/development/raylib/raylib/src/models.c:638
#3 0x55793597929f in main src/main.c:164
#4 0x7fa91d9f7dea in __libc_start_main ../csu/libc-start.c:308
looking further

[chriscamacho]

it's to do with material loading (on duplicating a string - which isn't there)
at first i thought it was because tinyobj always loads mtl's from the current directory instead of the directory that the obj is in. (hence I always tend to manually load textures for an obj...)
However the issue still seem to be there with a valid mtl / texture
alas it looks more complicated than I thought, I need to learn how the tinyobj code is doing things, and I'm a little pressed for time, let me know if you have any insights as I'm pressed for time just now...

[chriscamacho]

just before I dash off!
https://github.com/syoyo/tinyobjloader-c the example here doesn't have this issue... where are you getting tinyobjloader from is this version a possible fix ?

[raysan5]

@chriscamacho Just updated tinyobjloader-c to latest version on commit 53b32f1.

[chriscamacho]

TINYOBJ: Error reading file 'bridgehex.mtl': No such file or directory (2)
TINYOBJ: Failed to parse material file 'bridgehex.mtl': -3
INFO: [data/bridgehex.obj] Model data loaded successfully: 1 meshes / 0 materials

Thread 1 "hexRPG" received signal SIGSEGV, Segmentation fault.
0x00005555555e6c83 in LoadOBJ (fileName=0x555555603653 "data/bridgehex.obj")
at models.c:2846
2846 mesh.texcoords[vtCount + 0] = attrib.texcoords[idx0.vt_idx*2 + 0];

different error now! :)
this model loads okay (without mtl) into the view example with tinyobjloader-c

I'll steal some time and have a quick look at models.c

[chriscamacho]

tinyobj is now reporting 2322 verts for a model with 151 ! size_t issue ??

[raysan5]

@chriscamacho that's weird but I changed size_t last time I used... maybe un-optimized vertex data reported?

[chriscamacho]

that was only a guess, but its defiantly not reporting the correct number of verts...! I've only had time to have a quick look and its not something obvious alas...

[chriscamacho]

in static char *my_strndup(const char *s, unsigned int len) (tinyobl_loader.h)
strlen is causing the problem (not sure why) comment it out and set slen to len and everything seems to work (at least enough to get -fsanitize=address to work

also when you load the object you allocate some memory to load the obj text into data, but you later don't free data :-o

doing this work-a-round allow you to see some interesting leaks in raylib, for example unloading a model seems to leak a small part of the material....

there is also a small leak in loadshader

[raysan5]

Issue reviewed with commit e5d5f6e. Please let me know if it solves the issue.

[raysan5]

@chriscamacho I'm closing this issue, feel free to reopen if issue persists.

[chriscamacho]

yeah, think you nailed it, thanks....