256x1 image fails to decode (crash)

[jamie-pate]

Found on godot engine when bcdec was integrated..
godotengine/godot#98902

The test program crashes while decoding the following test image:
256x1_test_image.zip

Starting program: test ./test_images/256x1_bc3.dds
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Successfully loaded ./test_images/256x1_bc3.dds
 w = 256, h = 1, format = BC3
Writing output to ./test_images/256x1_bc3.tga
malloc(): invalid size (unsorted)

Program received signal SIGABRT, Aborted.
__pthread_kill_implementation (no_tid=0, signo=6, threadid=140737353721664) at ./nptl/pthread_kill.c:44
44	./nptl/pthread_kill.c: No such file or directory.
(gdb) bt
#0  __pthread_kill_implementation (no_tid=0, signo=6, threadid=140737353721664) at ./nptl/pthread_kill.c:44
#1  __pthread_kill_internal (signo=6, threadid=140737353721664) at ./nptl/pthread_kill.c:78
#2  __GI___pthread_kill (threadid=140737353721664, signo=signo@entry=6) at ./nptl/pthread_kill.c:89
#3  0x00007ffff7c42476 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#4  0x00007ffff7c287f3 in __GI_abort () at ./stdlib/abort.c:79
#5  0x00007ffff7c89676 in __libc_message (action=action@entry=do_abort, fmt=fmt@entry=0x7ffff7ddbb77 "%s\n") at ../sysdeps/posix/libc_fatal.c:155
#6  0x00007ffff7ca0cfc in malloc_printerr (str=str@entry=0x7ffff7ddebc0 "malloc(): invalid size (unsorted)") at ./malloc/malloc.c:5664
#7  0x00007ffff7ca40dc in _int_malloc (av=av@entry=0x7ffff7e1ac80 <main_arena>, bytes=bytes@entry=4096) at ./malloc/malloc.c:4002
#8  0x00007ffff7ca5262 in __GI___libc_malloc (bytes=bytes@entry=4096) at ./malloc/malloc.c:3321
#9  0x00007ffff7c7eba4 in __GI__IO_file_doallocate (fp=0x5555555662a0) at ./libio/filedoalloc.c:101
#10 0x00007ffff7c8dce0 in __GI__IO_doallocbuf (fp=fp@entry=0x5555555662a0) at ./libio/libioP.h:947
#11 0x00007ffff7c8cf60 in _IO_new_file_overflow (f=0x5555555662a0, ch=-1) at ./libio/fileops.c:744
#12 0x00007ffff7c8b6d5 in _IO_new_file_xsputn (n=1, data=<optimized out>, f=<optimized out>) at ./libio/libioP.h:947
#13 _IO_new_file_xsputn (f=0x5555555662a0, data=<optimized out>, n=1) at ./libio/fileops.c:1196
#14 0x00007ffff7c7ffd7 in __GI__IO_fwrite (buf=0x7fffffffd6bb, size=1, count=1, fp=0x5555555662a0) at ./libio/libioP.h:947
#15 0x00005555555553e0 in stbi.stdio_write ()
#16 0x000055555555556a in stbiw.writefv ()
#17 0x0000555555555784 in stbiw.writef ()
#18 0x00005555555561cb in stbi_write_tga_core ()
#19 0x0000555555556548 in stbi_write_tga ()
#20 0x000055555555f66a in main ()

The issue on godot is a bit different,

double free or corruption (!prev)
Aborted

[jamie-pate]

gcc ./test.c -O0 -o test -ggdb -fsanitize=address

Successfully loaded ./test_images/256x1_bc3.dds
 w = 256, h = 1, format = BC3
=================================================================
==3281157==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000000018 at pc 0x57d9d2f81f1d bp 0x7fff65a662b0 sp 0x7fff65a662a0
READ of size 2 at 0x602000000018 thread T0
    #0 0x57d9d2f81f1c in bcdec__color_block bcdec.h:119
    #1 0x57d9d2f83170 in bcdec_bc3 bcdec.h:287
    #2 0x57d9d2f8d621 in main test.c:201
    #3 0x73e05dc29d8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #4 0x73e05dc29e3f in __libc_start_main_impl ../csu/libc-start.c:392
    #5 0x57d9d2f72564 in _start (/home/jpate/build/src/bcdec/test+0x4564)

0x602000000018 is located 7 bytes to the right of 1-byte region [0x602000000010,0x602000000011)
allocated by thread T0 here:
    #0 0x73e05e0b4887 in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:145
    #1 0x57d9d2f8cda4 in load_dds test.c:134
    #2 0x57d9d2f8d197 in main test.c:177
    #3 0x73e05dc29d8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58

SUMMARY: AddressSanitizer: heap-buffer-overflow bcdec.h:119 in bcdec__color_block
Shadow bytes around the buggy address:
  0x0c047fff7fb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c047fff7fc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c047fff7fd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c047fff7fe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c047fff7ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c047fff8000: fa fa 01[fa]fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff8010: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff8020: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff8030: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff8040: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff8050: 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
==3281157==ABORTING

[jamie-pate]

#define BCDEC_BC3_COMPRESSED_SIZE(w, h) ((((w)>>2)*((h)>>2))*BCDEC_BC3_BLOCK_SIZE)

1 >> 2 == 0

[jamie-pate]

I found two problems with the program which both cause buffer overruns..

1. BCDEC_BCX_COMPRESSED_SIZE macros will all evalutate to 0 if h or w < 4, causing the compressed image data to be allocated 0 bytes of memory...

This could be fixed by adding a BCDEC_MAX1 macro

#define BCDEC_MAX1(i) (i < 1 ? 1 : i)
...
#define BCDEC_BC3_COMPRESSED_SIZE(w, h)     (((BCDEC_MAX1((w)>>2))*(BCDEC_MAX1((h)>>2)))*BCDEC_BC3_BLOCK_SIZE)

2. The test program only allocates w * h * 4 bytes for uncompressed data, but bcdec_bc3() will use 4 * destinationPitch + 4 * 4 bytes to decompress the color block no matter the image size bcdec__color_block() here

[iOrange]

Hi!

The test app is just a simple use case to demonstrate the bcdec API and for me to test and profile the library.
The bcdec API was designed to be as minimalistic as possible while being feature-complete.

The BC format operates on blocks of size 4, and so every function expects input as an array of 4x4 and same for the output.
It is user's responsibility to make sure both input and output memory is of sufficient size.

As for the issue in Godot - it should be addressed by the person who integrated the library, and it looks like that person already addressed this in this commit - godotengine/godot@a60195e.

[jamie-pate]

Thanks, I feel the example code should test the edge cases as well though. The person who integrates the library won't know about them if you don't put them in the example test code :D

I ended up spending most of my day trying to figure this out, some comments for the function definitions specifying their requirements explicitly would go a long way. (I did learn a lot about BC decoding though along the way)

[iOrange]

For sure I can add this to the test app, but to be completely honest - this is specifically mentioned in the README of the library

BC1/BC2/BC3/BC7 are expected to decompress into 4*4 RGBA blocks 8bit per component (32bit pixel)

But anyways, I'll think on how to make it even more obvious thanks.