satoshinm / NetCraft

Web-based fork of fogleman/Craft ⛺
https://satoshinm.github.io/NetCraft/
MIT License
57 stars 13 forks source link

Enable sanitizers in debug build #124

Closed satoshinm closed 7 years ago

satoshinm commented 7 years ago

To help catch bugs

satoshinm commented 7 years ago

Already finding bugs:

native-build $ ./craft 
/Users/admin/games/wasm/NetCraft/deps/lodepng/lodepng.c:330:21: runtime error: left shift of 244 by 24 places cannot be represented in type 'int'
SUMMARY: AddressSanitizer: undefined-behavior /Users/admin/games/wasm/NetCraft/deps/lodepng/lodepng.c:330:21 in 
/Users/admin/games/wasm/NetCraft/deps/sqlite/sqlite3.c:62765:18: runtime error: left shift of 192 by 24 places cannot be represented in type 'int'
SUMMARY: AddressSanitizer: undefined-behavior /Users/admin/games/wasm/NetCraft/deps/sqlite/sqlite3.c:62765:18 in 
/Users/admin/games/wasm/NetCraft/deps/sqlite/sqlite3.c:62766:18: runtime error: left shift of 128 by 24 places cannot be represented in type 'int'
SUMMARY: AddressSanitizer: undefined-behavior /Users/admin/games/wasm/NetCraft/deps/sqlite/sqlite3.c:62766:18 in 
/Users/admin/games/wasm/NetCraft/deps/sqlite/sqlite3.c:59412:22: runtime error: member access within null pointer of type 'Mem' (aka 'struct Mem')
SUMMARY: AddressSanitizer: undefined-behavior /Users/admin/games/wasm/NetCraft/deps/sqlite/sqlite3.c:59412:22 in 
/Users/admin/games/wasm/NetCraft/src/map.c:11:15: runtime error: signed integer overflow: 9740386 * 2057 cannot be represented in type 'int'
SUMMARY: AddressSanitizer: undefined-behavior /Users/admin/games/wasm/NetCraft/src/map.c:11:15 in 
/Users/admin/games/wasm/NetCraft/src/map.c:7:23: runtime error: left shift of negative value -225
SUMMARY: AddressSanitizer: undefined-behavior /Users/admin/games/wasm/NetCraft/src/map.c:7:23 in 
/Users/admin/games/wasm/NetCraft/deps/sqlite/sqlite3.c:62726:41: runtime error: left shift of negative value -1
SUMMARY: AddressSanitizer: undefined-behavior /Users/admin/games/wasm/NetCraft/deps/sqlite/sqlite3.c:62726:41 in 
/Users/admin/games/wasm/NetCraft/src/main.c:838:14: runtime error: index 8 out of bounds for type 'const int [8]'
SUMMARY: AddressSanitizer: undefined-behavior /Users/admin/games/wasm/NetCraft/src/main.c:838:14 in 
=================================================================
==85100==ERROR: AddressSanitizer: global-buffer-overflow on address 0x000100b0bb80 at pc 0x0001006ad09a bp 0x7fff5f571390 sp 0x7fff5f571388
READ of size 4 at 0x000100b0bb80 thread T0
    #0 0x1006ad099 in _gen_sign_buffer main.c:838
    #1 0x1006ad791 in gen_sign_buffer main.c:899
    #2 0x1006b9d69 in check_workers main.c:1350
    #3 0x1006bbeb5 in ensure_chunks main.c:1484
    #4 0x1006bd621 in render_chunks main.c:1679
    #5 0x1006d1486 in render_scene main.c:3223
    #6 0x1006d0e4f in one_iter main.c:3187
    #7 0x1006cfa84 in main main.c:3011
    #8 0x7fff94cae234 in start (libdyld.dylib:x86_64+0x5234)

0x000100b0bb80 is located 32 bytes to the left of global variable 'glyph_dz' defined in '/Users/admin/games/wasm/NetCraft/src/main.c:823:22' (0x100b0bba0) of size 32
0x000100b0bb80 is located 0 bytes to the right of global variable 'glyph_dx' defined in '/Users/admin/games/wasm/NetCraft/src/main.c:822:22' (0x100b0bb60) of size 32
SUMMARY: AddressSanitizer: global-buffer-overflow main.c:838 in _gen_sign_buffer
Shadow bytes around the buggy address:
  0x100020161720: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100020161730: 00 00 00 00 f9 f9 f9 f9 00 00 00 00 00 00 00 00
  0x100020161740: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100020161750: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100020161760: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x100020161770:[f9]f9 f9 f9 00 00 00 00 f9 f9 f9 f9 00 00 00 00
  0x100020161780: f9 f9 f9 f9 00 00 00 00 f9 f9 f9 f9 00 00 00 00
  0x100020161790: f9 f9 f9 f9 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000201617a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000201617b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000201617c0: f9 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 00 00 00 00
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
==85100==ABORTING
Abort trap: 6
satoshinm commented 7 years ago

From lodepng, this is giving left shift of 244 by 24 places cannot be represented in type 'int':

unsigned lodepng_read32bitInt(const unsigned char* buffer)
{
  return (buffer[0] << 24) | (buffer[1] << 16) | (buffer[2] << 8) | buffer[3];
}

but how is it undefined? 244 << 24 is 0xf4000000 and that fits in an unsigned int (not int, why does it say int?). Next 3 errors are similar left shifts by 24. All sanitizer documentation: http://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html. Hint at https://github.com/nothings/stb/issues/152#issuecomment-137532508:

I think maybe get8(f) << 24, it's promoting the uint8 on the left to int instead of unsigned int.

satoshinm commented 7 years ago

Good, it found a real bug from https://github.com/satoshinm/NetCraft/pull/112 - fixed. Suppress the shifting errors in lodepng and sqlite since they are dependencies, not code going to fix and seems harmless. Remaining runtime error is mysterious:

NetCraft/deps/sqlite/sqlite3.c:59413:22: runtime error: member access within null pointer of type 'Mem' (aka 'struct Mem')
NetCraft/deps/sqlite/sqlite3.c:59413:22 in 

on sqlite:

SQLITE_PRIVATE void sqlite3VdbeMemShallowCopy(Mem *pTo, const Mem *pFrom, int srcType){
  assert( (pFrom->flags & MEM_RowSet)==0 );
  VdbeMemRelease(pTo);
  memcpy(pTo, pFrom, MEMCELLSIZE); // here
  pTo->xDel = 0;
  if( (pFrom->flags&MEM_Static)==0 ){
    pTo->flags &= ~(MEM_Dyn|MEM_Static|MEM_Ephem);
    assert( srcType==MEM_Ephem || srcType==MEM_Static );
    pTo->flags |= srcType;
  }
}

but if a compile a check to test for (!pTo || !pFrom), it isn't hit. Breaking in the debugger:

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = ubsan-issue
  * frame #0: 0x0000000100c28660 libclang_rt.asan_osx_dynamic.dylib`__ubsan_on_report
    frame #1: 0x0000000100c23adb libclang_rt.asan_osx_dynamic.dylib`__ubsan::Diag::~Diag() + 155
    frame #2: 0x0000000100c250e4 libclang_rt.asan_osx_dynamic.dylib`handleTypeMismatchImpl(__ubsan::TypeMismatchData*, unsigned long, __ubsan::ReportOptions) + 628
    frame #3: 0x0000000100c24e64 libclang_rt.asan_osx_dynamic.dylib`__ubsan_handle_type_mismatch + 68
    frame #4: 0x0000000100246bea craft`sqlite3VdbeMemShallowCopy(pTo=0x000061700001bd00, pFrom=0x0000619000080f38, srcType=2048) at sqlite3.c:59413 [opt]
    frame #5: 0x0000000100205cc7 craft`sqlite3VdbeExec(p=0x000061200005b0c0) at sqlite3.c:66341 [opt]
    frame #6: 0x0000000100115a1c craft`sqlite3_step [inlined] sqlite3Step at sqlite3.c:63612 [opt]
    frame #7: 0x000000010011561f craft`sqlite3_step(pStmt=<unavailable>) at sqlite3.c:63678 [opt]
    frame #8: 0x0000000100010909 craft`db_load_signs(list=0x000000010077e188, p=3, q=-7) at db.c:448 [opt]
    frame #9: 0x000000010002bfc1 craft`init_chunk(chunk=0x000000010077e148, p=3, q=<unavailable>) at main.c:1266 [opt]
    frame #10: 0x000000010002c333 craft`create_chunk(chunk=<unavailable>, p=3, q=-7) at main.c:1277 [opt]
    frame #11: 0x000000010002d6c2 craft`force_chunks(player=<unavailable>) at main.c:1389 [opt]
    frame #12: 0x000000010004356c craft`main_inited at main.c:3096 [opt]
    frame #13: 0x0000000100042d95 craft`main(argc=<unavailable>, argv=<unavailable>) at main.c:3009 [opt]
    frame #14: 0x00007fff94cae235 libdyld.dylib`start + 1
(lldb) f 4
craft was compiled with optimization - stepping may behave oddly; variables may not be available.
frame #4: 0x0000000100246bea craft`sqlite3VdbeMemShallowCopy(pTo=0x000061700001bd00, pFrom=0x0000619000080f38, srcType=2048) at sqlite3.c:59413 [opt]
   59410      assert( (pFrom->flags & MEM_RowSet)==0 );
   59411      VdbeMemRelease(pTo);
   59412      if (!pTo || !pFrom) printf("pto=%p, pfrom=%p\n", pTo, pFrom);
-> 59413      memcpy(pTo, pFrom, MEMCELLSIZE);
   59414      pTo->xDel = 0;
   59415      if( (pFrom->flags&MEM_Static)==0 ){
   59416        pTo->flags &= ~(MEM_Dyn|MEM_Static|MEM_Ephem);
(lldb) 

Why does it think this is a type mismatch? Because pTo is Mem * but pFrom const Mem *?


It's actually the macro, this does dereference a null pointer, sigh:

/*
** Size of struct Mem not including the Mem.zMalloc member.
*/
#define MEMCELLSIZE (size_t)(&(((Mem *)0)->zMalloc))

but this diagnostic does hit other errors, another handleTypeMismatchImpl:

(lldb) f 4
craft was compiled with optimization - stepping may behave oddly; variables may not be available.
frame #4: 0x00000001000068b2 craft`client_connect(hostname=<unavailable>, port=<unavailable>) at client.c:244 [opt]
   241      }
   242      memset(&address, 0, sizeof(address));
   243      address.sin_family = AF_INET;
-> 244      address.sin_addr.s_addr = ((struct in_addr *)(host->h_addr_list[0]))->s_addr;
   245      address.sin_port = htons(port);
   246      if ((sd = socket(AF_INET, SOCK_STREAM, 0)) == -1) {
   247          perror("socket");