raylib: [core] Address Sanitizer crashes on `DecompressData()`, Global buffer overflow?

UPDATE 1: After further testing, the ASAN crash only happens when using a pre-defined buffer containing the font data compressed, added a zip file with the test code —>raylib_asan_issue.zip

UPDATE 2: As pointed on X thread, issue is probably related to sinfl reading chunks of 8 bytes and the size of data is 2286 that is not a multiple of 8 so 2 extra bytes get read out of bounds. Should it be maybe considered by sinfl?

Recently a crash was reported here. After some investigation and enabling ASAN, a crash is detected on DecompressData(). It only happens with ASAN enabled, without ASAN everything works as expected, data is decompressed and used successfully.

NOTE: To test it, the example and raylib (latest master branch) must be built with ASAN enabled.

The issue points to sinfl library, so I did some tests trying to find the issue. Here some testing code:

Simple compression/decompression test, everything works, no ASAN complaints.

#include "raylib.h"

#include <stdlib.h>
#include <stdio.h>

int main(void)
{
    char *data = (char *)RL_CALLOC(8*1024*1024, sizeof(char));

    SetRandomSeed(52713);
    for (int i = 0; i < 8*1024*1024; i += 1024) data[i] = (char)GetRandomValue(0, 255);

    int compDataSize = 0;
    char *compData = CompressData(data, 8*1024*1024, &compDataSize);

    printf("Data compressed: %i ---> %i bytes
[raylib_asan_issue.zip](https://github.com/raysan5/raylib/files/12735862/raylib_asan_issue.zip)
\n", 8*1024*1024, compDataSize);

    RL_FREE(data);
    data = NULL;

    int dataSize = 0;
    data = DecompressData(compData, compDataSize, &dataSize);

    printf("Data decompressed: %i ---> %i bytes\n", compDataSize, dataSize);

    return 0;
}

Testing a font image data compression/decompression, everything works, no ASAN complaints.

Image used: 512x256@16bytes (2 bytes per pixel) - Attached image: font_cyber

#include "raylib.h"

#include <stdlib.h>
#include <stdio.h>

int main(void)
{
    Image fontCyber = LoadImage("font_cyber.png");

    int compDataSize = 0;
    char *compData = CompressData(fontCyber.data, fontCyber.width*fontCyber.height*2, &compDataSize);

    printf("Data compressed: %i ---> %i bytes\n", fontCyber.width*fontCyber.height*2, compDataSize);

    int dataSize = 0;
    char *data = DecompressData(compData, compDataSize, &dataSize);     <--- ASAN crash!

    printf("Data decompressed: %i ---> %i bytes\n", compDataSize, dataSize);

    return 0;
}

Testing font image data provided as a global array decompression, crash ASAN complaints.

#include "raylib.h"

#include <stdlib.h>
#include <stdio.h>

#define FONT_ATLAS_COMP_SIZE 2286

// Font atlas image pixels data: DEFLATE compressed
static unsigned char fontData[FONT_ATLAS_COMP_SIZE] = { ... };

//------------------------------------------------------------------------------------
// Program main entry point
//------------------------------------------------------------------------------------
int main(void)
{
    int dataSize = 0;
    char *data = DecompressData(fontData, FONT_ATLAS_COMP_SIZE, &dataSize);

    printf("Data decompressed: %i ---> %i bytes\n", FONT_ATLAS_COMP_SIZE, dataSize);

    return 0;
}

ASAN crash output:

==10472==ERROR: AddressSanitizer: global-buffer-overflow on address 0x7ff6afaca1ee at pc 0x7ffe2c2718fb bp 0x00245e17cff0 sp 0x00245e17c780
READ of size 8 at 0x7ff6afaca1ee thread T0
==10472==WARNING: Failed to use and restart external symbolizer!
    #0 0x7ffe2c2718fa in _asan_wrap_GlobalSize+0x4e226 (C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.37.32822\bin\HostX64\x64\clang_rt.asan_dynamic-x86_64.dll+0x1800518fa)
    #1 0x7ff6af8fe11f in sinfl_read64 C:\GitHub\raylib\src\external\sinfl.h:183
    #2 0x7ff6af8fe64a in sinfl_refill C:\GitHub\raylib\src\external\sinfl.h:213
    #3 0x7ff6af901649 in sinfl_decompress C:\GitHub\raylib\src\external\sinfl.h:467
    #4 0x7ff6af8fdd7a in sinflate C:\GitHub\raylib\src\external\sinfl.h:570
    #5 0x7ff6af8d01e0 in DecompressData C:\GitHub\raylib\src\rcore.c:3613
    #6 0x7ff6af8c1116 in main C:\GitHub\raylib\examples\core\core_basic_window.c:188
    #7 0x7ff6afa2d548 in invoke_main D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:78
    #8 0x7ff6afa2d49d in __scrt_common_main_seh D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288
    #9 0x7ff6afa2d35d in __scrt_common_main D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:330
    #10 0x7ff6afa2d5bd in mainCRTStartup D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_main.cpp:16
    #11 0x7ffee5bc7343 in BaseThreadInitThunk+0x13 (C:\Windows\System32\KERNEL32.DLL+0x180017343)
    #12 0x7ffee63a26b0 in RtlUserThreadStart+0x20 (C:\Windows\SYSTEM32\ntdll.dll+0x1800526b0)

0x7ff6afaca1ee is located 0 bytes to the right of global variable 'cyberFontData' defined in 'core_basic_window.c:32:21' (0x7ff6afac9900) of size 2286
SUMMARY: AddressSanitizer: global-buffer-overflow (C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.37.32822\bin\HostX64\x64\clang_rt.asan_dynamic-x86_64.dll+0x1800518fa) in _asan_wrap_GlobalSize+0x4e226
Shadow bytes around the buggy address:
  0x1165e65d93e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1165e65d93f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1165e65d9400: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1165e65d9410: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1165e65d9420: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x1165e65d9430: 00 00 00 00 00 00 00 00 00 00 00 00 00[06]f9 f9
  0x1165e65d9440: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x1165e65d9450: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x1165e65d9460: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x1165e65d9470: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x1165e65d9480: 00 00 00 00 05 f9 f9 f9 f9 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
Address Sanitizer Error: Global buffer overflow

Just for completion, here it is the current code for CompressData() and DecompressData(), just in case I was doing some missuse of sinfl/sdefl libraries:

// Compress data (DEFLATE algorithm)
unsigned char *CompressData(const unsigned char *data, int dataSize, int *compDataSize)
{
    #define COMPRESSION_QUALITY_DEFLATE  8

    unsigned char *compData = NULL;

#if defined(SUPPORT_COMPRESSION_API)
    // Compress data and generate a valid DEFLATE stream
    struct sdefl *sdefl = RL_CALLOC(1, sizeof(struct sdefl));   // WARNING: Possible stack overflow, struct sdefl is almost 1MB
    int bounds = sdefl_bound(dataSize);
    compData = (unsigned char *)RL_CALLOC(bounds, 1);
    
    *compDataSize = sdeflate(sdefl, compData, data, dataSize, COMPRESSION_QUALITY_DEFLATE);   // Compression level 8, same as stbiw
    RL_FREE(sdefl);
    
    TRACELOG(LOG_INFO, "SYSTEM: Compress data: Original size: %i -> Comp. size: %i", dataSize, *compDataSize);
#endif

    return compData;
}

// Decompress data (DEFLATE algorithm)
unsigned char *DecompressData(const unsigned char *compData, int compDataSize, int *dataSize)
{
    unsigned char *data = NULL;

#if defined(SUPPORT_COMPRESSION_API)
    // Decompress data from a valid DEFLATE stream
    data = (unsigned char *)RL_CALLOC(MAX_DECOMPRESSION_SIZE*1024*1024, 1);
    int tempDataSize = sinflate(data, MAX_DECOMPRESSION_SIZE*1024*1024, compData, compDataSize);

    // WARNING: RL_REALLOC can make (and leave) data copies in memory, be careful with sensitive compressed data!
    // TODO: Use a different approach, create another buffer, copy data manually to it and wipe original buffer memory
    unsigned char *temp = (unsigned char *)RL_REALLOC(data, tempDataSize);

    if (temp != NULL) data = temp;
    else TRACELOG(LOG_WARNING, "SYSTEM: Failed to re-allocate required decompression memory");

    *dataSize = tempDataSize;

    TRACELOG(LOG_INFO, "SYSTEM: Decompress data: Comp. size: %i -> Original size: %i", compDataSize, *dataSize);
#endif

    return data;
}

Any help is really welcome, I’ve been reviewing it for hours but I couldn’t find the source of the issue…

About this issue

Original URL
State: closed
Created 9 months ago
Comments: 25 (8 by maintainers)

Commits related to this issue

REVIEWED: `sinfl` external library to avoid ASAN complaints #3349 — committed to raysan5/raylib by raysan5 8 months ago

Most upvoted comments

Here are few unchecked cases, hope it’s going to be useful to the sinfl’s developer.

L415: only input buffer checked, but not the output, this will write out of bounds when out + len >= oe. L477: this will write out of bounds when out + 1 == oe. L508: this will read out of bounds, and even write out of bounds if oe - out == 49; len, offs or out - dst isn’t multiple of 16. L514: same issue, just not being multiple of 8. L525: this might read out of bounds, and definitely will write out of bounds when dst >= out and dst + 3 <= out. L558: same issue, only this time no oe > out checks of any kind. The set in line 500 could have moved out beyond oe, and this block would still write at least 3 bytes happily.

Hope this is useful and helps, bzt

bztsrc on Sep 29, 2023

Hmmm, I take it back. All the other sinfl_copy64 etc. functions are problematic too (they are called within a badly formatted loop, that’s why I haven’t noticed at first). All these functions will guaranteed to read out of bounds or write out of bounds, and should be fixed by adding a similar “bitptr + 8 < bitend” check.

Also, sinfl_copy64 wastes stack without any reason, and it’s inefficient. It should be just simply

sinfl_copy64(unsigned char **dst, unsigned char **src) {
-  unsigned long long n;
-  memcpy(&n, *src, 8);
-  memcpy(*dst, &n, 8);
+  memcpy(*dst, *src, 8);
   *dst += 8, *src += 8;
}

(plus the bound check of course)

Hope this helps, bzt

bztsrc on Sep 27, 2023

@raysan5: I see. It was just an idea.

@r-lyeh: it’s not 16 excess bytes, but 8, and either way won’t work. As @raysan5 pointed out, he have tried and it indeed doesn’t work. (The problem here is, if you increase the buffer size to be multiple of something, then you also add more bytes to decompress, and sinfl does not guarantee that it only reads on 8 bytes boundaris, so you can’t just round up the size anyway)

The real solution is not to read out of bounds. Here’s a proper fix:

diff --git a/src/external/sinfl.h b/src/external/sinfl.h
index 8979fcd7..3a774436 100644
--- a/src/external/sinfl.h
+++ b/src/external/sinfl.h
@@ -122,6 +122,7 @@ extern "C" {
 
 struct sinfl {
   const unsigned char *bitptr;
+  const unsigned char *bitend;
   unsigned long long bitbuf;
   int bitcnt;
 
@@ -177,12 +178,6 @@ sinfl_bsr(unsigned n) {
   return 31 - __builtin_clz(n);
 #endif
 }
-static unsigned long long
-sinfl_read64(const void *p) {
-  unsigned long long n;
-  memcpy(&n, p, 8);
-  return n;
-}
 static void
 sinfl_copy64(unsigned char **dst, unsigned char **src) {
   unsigned long long n;
@@ -210,7 +205,9 @@ sinfl_copy128(unsigned char **dst, unsigned char **src) {
 #endif
 static void
 sinfl_refill(struct sinfl *s) {
-  s->bitbuf |= sinfl_read64(s->bitptr) << s->bitcnt;
+  unsigned long long n = 0;
+  memcpy(&n, p, s->bitptr + 8 < s->bitend ? 8 : s->bitend - s->bitptr);
+  s->bitbuf |= n << s->bitcnt;
   s->bitptr += (63 - s->bitcnt) >> 3;
   s->bitcnt |= 56; /* bitcount in range [56,63] */
 }
@@ -384,6 +381,7 @@ sinfl_decompress(unsigned char *out, int cap, const unsigned char *in, int size)
   int last = 0;
 
   s.bitptr = in;
+  s.bitend = e;
   while (1) {
     switch (state) {
     case hdr: {

Few notes:

the buffer end pointer has to be added to the structure, because currently it’s just a local variable in sinfl_decomress, so the other functions have no clue where the buffer ends, which is VERY bad.
I’ve removed sinfl_read64 and inlined it instead, because it was called at one place only and was just a wrapper around another function call (unnecessary waste of stack), and now sinfl_refill also checks the remaining buffer size.
The n local variable must be initialized as 0, because this time memcpy might copy less than 8 bytes, however n will still act as if it were read entirely from the buffer.
There’s no issue with the other functions (like sinfl_copy64 or sinfl_write64), because it looks like they are only called inside properly guarded if blocks.
Another thing that might be worth considering, that using memcpy like this isn’t endian-safe. It won’t work across different endian machines, and I’m not sure if this implementation is RFC1951 compliant at all. (However this is not an issue if all raylib platforms are little-endian and supporting a big-endian platform isn’t planned in the future. Not far fetched, all modern architectures are little-endian anyway.)

Cheers, bzt

bztsrc on Sep 27, 2023