Memory corruption on test GIF

[wcout]

Is there a problem with this GIF?

https://github.com/robert-ancell/pygif/blob/master/test-suite/extra-pixels.gif

The following simple program crashes here with a memory corruption:

#include <cstdio>

#include "gif_load.h"

void frame_cb( void *data_, GIF_WHDR *whdr_ ) {
  printf( "frame_cb: %s, %p  frame #%ld/%ld, %ld x %ld, delay: %ld\n",
    (char *)data_, (void *)whdr_, whdr_->ifrm, whdr_->nfrm,
     whdr_->frxd, whdr_->fryd, whdr_->time );
}

int main( int argc_, char *argv_[] ) {
  if ( argc_ <= 1 ) return 0;
  printf( "load %s\n", argv_[1] );
  FILE *gif = fopen( argv_[1], "r" );
  long len = 0;
  char *buf = 0;
  if ( !( gif && fseek( gif, 0, SEEK_END ) >= 0 &&
        ( len = ftell( gif ) ) >= 0             &&
        ( buf = (char *)malloc( (size_t)len ) ) &&
        fseek( gif, 0, SEEK_SET ) >= 0          &&
        fread( buf, 1, (size_t)len, gif ) == (size_t)len ) ) {
    perror( argv_[1] );
    free( buf );
    if ( gif ) fclose( gif );
    return 1;
  }
  fclose( gif );

  printf( "decode '%s', buf = %p, len = %lu\n", argv_[1], buf, len );
  long ret = GIF_Load( buf, len, frame_cb, 0, argv_[1], 0 );
  printf( "ret = %ld\n", ret );

  return 0;
}

output:

load ../pygif/test-suite/extra-pixels.gif
decode '../pygif/test-suite/extra-pixels.gif', buf = 0x558f4942e8b0, len = 60
frame_cb: ../pygif/test-suite/extra-pixels.gif, 0x7fff97f3d480  frame #0/1, 1 x 1, delay: 0
free(): invalid next size (normal)

Strangely the demo program (from README) outputs the tgacorrectly and does not crash.

This is with Ubuntu 18.10.

[wcout]

g++ -fsanitize=address -o example example.c -g
~:$ ./example ../pygif/test-suite/extra-pixels.gif
load ../pygif/test-suite/extra-pixels.gif
decode '../pygif/test-suite/extra-pixels.gif', buf = 0x606000000020, len = 60
=================================================================
==17228==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x629000004201 at pc 0x55d98e37e0c4 bp 0x7ffd51cb55d0 sp 0x7ffd51cb55c0
WRITE of size 1 at 0x629000004201 thread T0
    #0 0x55d98e37e0c3 in _GIF_LoadFrame gif_load.h:136
    #1 0x55d98e37f5d6 in GIF_Load gif_load.h:260
    #2 0x55d98e380960 in main example.c:30
    #3 0x7fddde9ba09a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
    #4 0x55d98e37d209 in _start (example+0x1209)

0x629000004201 is located 0 bytes to the right of 16385-byte region [0x629000000200,0x629000004201)
allocated by thread T0 here:
    #0 0x7fdddec6df30 in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.5+0xedf30)
    #1 0x55d98e37f1c4 in GIF_Load gif_load.h:248
    #2 0x55d98e380960 in main example.c:30
    #3 0x7fddde9ba09a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)

SUMMARY: AddressSanitizer: heap-buffer-overflow gif_load.h:136 in _GIF_LoadFrame
Shadow bytes around the buggy address:
  0x0c527fff87f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c527fff8800: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c527fff8810: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c527fff8820: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c527fff8830: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c527fff8840:[01]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c527fff8850: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c527fff8860: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c527fff8870: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c527fff8880: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c527fff8890: 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
==17228==ABORTING

[hidefromkgb]

Yeah, that GIF is incorrect. Its pixel stream exceeds its frame capacity (hence the name: extra-pixels.gif), thus creating a memory corruption as gif_load is trying desperately to shove excess pixels into the frame.

Fixed in 0ed5be0a2de7146dde6b0b7cb647ee8309aa86f5. Please verify.

[wcout]

Yes, this one is fixed with the latest commit.

I checked the whole testsuite now from the above mentioned location, and there is one remaining (corruption) issue with:

https://github.com/robert-ancell/pygif/blob/master/test-suite/image-zero-width.gif

./example ../pygif/test-suite/gl_failed/image-zero-width.gif
load ../pygif/test-suite/gl_failed/image-zero-width.gif
decode '../pygif/test-suite/gl_failed/image-zero-width.gif', buf = 0x603000000010, len = 30
=================================================================
==18205==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60300000002e at pc 0x5633078ceca1 bp 0x7ffccf85bd20 sp 0x7ffccf85bd10
READ of size 1 at 0x60300000002e thread T0
    #0 0x5633078ceca0 in GIF_Load gif_load.h:236
    #1 0x5633078d0af1 in main example.cpp:32
    #2 0x7fc280eb409a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
    #3 0x5633078cd219 in _start (example+0x1219)

0x60300000002e is located 0 bytes to the right of 30-byte region [0x603000000010,0x60300000002e)
allocated by thread T0 here:
    #0 0x7fc281167f30 in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.5+0xedf30)
    #1 0x5633078d0974 in main example.cpp:22
    #2 0x7fc280eb409a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)

SUMMARY: AddressSanitizer: heap-buffer-overflow gif_load.h:236 in GIF_Load
Shadow bytes around the buggy address:
  0x0c067fff7fb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c067fff7fc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c067fff7fd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c067fff7fe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c067fff7ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c067fff8000: fa fa 00 00 00[06]fa fa fa fa fa fa fa fa fa fa
  0x0c067fff8010: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c067fff8020: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c067fff8030: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c067fff8040: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c067fff8050: 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

[hidefromkgb]

Happens to be fixed in 74b674de2704bc1b20fc3bf482a5ee3e774b67c5.

[wcout]

Wow, that's quick! Confirmed. That whole testsuite now runs through.. Thank you very much.

(But will probably be back with a request soon)

[hidefromkgb]

You`re welcome =) BTW, could you please share your testing wrapper? It`d be nice to have one to run the suite once I change something.

[wcout]

Well, my testing wrapper is just the example program from above, expanded to handle multiple files and with better output. It's no logic test, but only a test if all files load without crashing anything. (i.e. it cannot test if gif_load thinks a file is invalid an returns 0, but in reality is a valid GIF format).

So if you run it (compiled with adress sanitizing) on a bunch of files and it does not bail out, then these will at least load.

test_gif_load.zip

One problem is, that gif_loadcurrently does not discern between invalid GIF files and files that are no GIF files at all. It would be nice to device a way, to return this information somehow.

[hidefromkgb]

Ah, OK. I thought you might have replicated the original parser to read the suite .conf files and do what`s written there. Closing this ticket as fixed, lest it goes off-topic. For a feature request please file a new ticket.