Heap Use After Free (allocated & freed in Undo stack)

[prust]

@guillaumechereau: Thank you for writing, maintaining & open-sourcing this amazing tool!

My son has been using it heavily and has been experiencing regular crashes. I compiled the latest master (292588c) in debug mode in hopes that that would produce helpful debugging information:

==6741==ERROR: AddressSanitizer: heap-use-after-free on address 0x510000001640 at pc 0x63ec2357423e bp 0x7ffcfccfbc40 sp 0x7ffcfccfbc30
READ of size 184 at 0x510000001640 thread T0
    #0 0x63ec2357423d in render_volume src/render.c:705
    #1 0x63ec22d0d890 in goxel_render_view src/goxel.c:1164
    #2 0x63ec22d091a1 in goxel_render src/goxel.c:1015
    #3 0x63ec231af8e9 in loop_function src/main.c:214
    #4 0x63ec231af9c4 in start_main_loop src/main.c:226
    #5 0x63ec231b167f in main src/main.c:448
    #6 0x7823fdc2a1c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #7 0x7823fdc2a28a in __libc_start_main_impl ../csu/libc-start.c:360
    #8 0x63ec22b58ad4 in _start (/home/peter/repos/goxel/goxel+0x14b0ad4) (BuildId: dce630ba01ac616fa233709905feced30f96cfc8)

0x510000001640 is located 0 bytes inside of 184-byte region [0x510000001640,0x5100000016f8)
freed by thread T0 here:
    #0 0x7823ffafa678 in free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:52
    #1 0x63ec231bff0d in material_delete src/material.c:39
    #2 0x63ec22d6d101 in image_restore src/image.c:189
    #3 0x63ec22d8492a in image_undo src/image.c:664
    #4 0x63ec22d1768d in undo src/goxel.c:1871
    #5 0x63ec22b61354 in action_exec src/action.c:100
    #6 0x63ec22d2af6e in check_action_shortcut src/gui.cpp:500
    #7 0x63ec22b61039 in actions_iter src/action.c:85
    #8 0x63ec22d3160e in gui_iter src/gui.cpp:692
    #9 0x63ec22d3177a in gui_render src/gui.cpp:702
    #10 0x63ec22d09244 in goxel_render src/goxel.c:1019
    #11 0x63ec231af8e9 in loop_function src/main.c:214
    #12 0x63ec231af9c4 in start_main_loop src/main.c:226
    #13 0x63ec231b167f in main src/main.c:448
    #14 0x7823fdc2a1c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #15 0x7823fdc2a28a in __libc_start_main_impl ../csu/libc-start.c:360
    #16 0x63ec22b58ad4 in _start (/home/peter/repos/goxel/goxel+0x14b0ad4) (BuildId: dce630ba01ac616fa233709905feced30f96cfc8)

previously allocated by thread T0 here:
    #0 0x7823ffafbb37 in malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69
    #1 0x63ec231bff34 in material_copy src/material.c:44
    #2 0x63ec22d6e6e8 in image_restore src/image.c:215
    #3 0x63ec22d8492a in image_undo src/image.c:664
    #4 0x63ec22d1768d in undo src/goxel.c:1871
    #5 0x63ec22b61354 in action_exec src/action.c:100
    #6 0x63ec22d2af6e in check_action_shortcut src/gui.cpp:500
    #7 0x63ec22b61039 in actions_iter src/action.c:85
    #8 0x63ec22d3160e in gui_iter src/gui.cpp:692
    #9 0x63ec22d3177a in gui_render src/gui.cpp:702
    #10 0x63ec22d09244 in goxel_render src/goxel.c:1019
    #11 0x63ec231af8e9 in loop_function src/main.c:214
    #12 0x63ec231af9c4 in start_main_loop src/main.c:226
    #13 0x63ec231b167f in main src/main.c:448
    #14 0x7823fdc2a1c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #15 0x7823fdc2a28a in __libc_start_main_impl ../csu/libc-start.c:360
    #16 0x63ec22b58ad4 in _start (/home/peter/repos/goxel/goxel+0x14b0ad4) (BuildId: dce630ba01ac616fa233709905feced30f96cfc8)

SUMMARY: AddressSanitizer: heap-use-after-free src/render.c:705 in render_volume
Shadow bytes around the buggy address:
  0x510000001380: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x510000001400: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x510000001480: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x510000001500: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x510000001580: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x510000001600: fa fa fa fa fa fa fa fa[fd]fd fd fd fd fd fd fd
  0x510000001680: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa
  0x510000001700: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x510000001780: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x510000001800: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x510000001880: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
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
==6741==ABORTING

In summary, it looks like the material that was originally allocated in image_undo() -> image_restore() -> material_copy(), but was later freed in image_undo() -> image_restore() -> material_delete(), was -- for some reason -- referenced in render_volume().

I've looked through image_restore() but haven't been able to spot the issue there. I'm wondering if maybe he's doing something else, between the undos, that might be triggering the bug.

He's able to reproduce the crash regularly, just by working in goxel for 10 minutes or so, but doesn't have specific repro steps. I haven't been successful at producing minimal repro steps either, though I've tried undoing and redoing a lot.

Would it produce more helpful logs if I recompile with GOX_LOG_VERBOSE? In debug mode, there isn't anything logged after startup, besides the occasional "Save" / "Saved" logs. Can you think of any other diagnostics I could do, or places I could look in the code to help me spot the issue? (I'm not very familiar with manual memory management but am trying to get better at it)

Update: I forgot to mention that I'm running in Linux Mint Cinnamon on an old iMac. To compile in debug mode, I added this to the Makefile:

debug:
    scons $(JOBS) mode=debug

[guillaumechereau]

Thanks for the report (and the stack!).

No need to use verbose, in debug you should already have all the logs.

Are using the current git master branch version? Your stack lines number don't seem to match the current code exactly.

I think the issue might come from the current error prone way we use a pointer for layer->material, that needs to point to an actual material of the image, and not to one in the undo stack. I suspect at some point in the code this constraint is broken (maybe in goxel_get_render_layers?)

Now I am thinking that I should replace those pointers with some sort of index to simplify.

[guillaumechereau]

I was able to reproduce a similar crash by opening an empty image on top of an empty image. I push a fix for that, that might also fix this crash as well.

[prust]

@guillaumechereau:

Are using the current git master branch version? Your stack lines number don't seem to match the current code exactly.

I did make one commit on top of master that touches a few files, the diff is here. I'm planning to open a pull request to see if you're interested. It introduces an autosave feature, which my son appreciated, given the crashes. But I thought I should make the autosave interval configurable (right now it always autosaves every 3 minutes), and I haven't done that yet.

push a fix for that, that might also fix this crash as well

Thank you, I'll give it a shot!

[prust]

@guillaumechereau: I pulled in your fix and last night my son used Goxel for over half an hour and didn't experience any crashes, so it may have fixed the issue. Thanks again!

[prust]

@guillaumechereau: My son used Goxel for hours since this fix (e875881c4) and hasn't had a crash, so I'll close this issue as Completed. Thank you again so much!