KelvinShadewing / brux-gdk

Free runtime and development kit using SDL and Squirrel
GNU Affero General Public License v3.0
39 stars 20 forks source link

deleteSprite() causes errors #34

Closed KelvinShadewing closed 1 year ago

KelvinShadewing commented 2 years ago

Using deleteSprite() will sometimes delete the wrong image. Here's the Valgrind output:

==447374== Memcheck, a memory error detector
==447374== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==447374== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
==447374== Command: brux tux.brx -f
==447374== 

/========================\
| BRUX GAME RUNTIME LOG |
\========================/

Initializing program...

==447374== Invalid read of size 8
==447374==    at 0x401D604: strncmp (strcmp.S:175)
==447374==    by 0x400604D: is_dst (dl-load.c:209)
==447374==    by 0x4008566: _dl_dst_count (dl-load.c:246)
==447374==    by 0x4008757: expand_dynamic_string_token (dl-load.c:388)
==447374==    by 0x40088D1: fillin_rpath.isra.0 (dl-load.c:460)
==447374==    by 0x4008BE1: decompose_rpath (dl-load.c:631)
==447374==    by 0x400974D: cache_rpath (dl-load.c:673)
==447374==    by 0x400974D: cache_rpath (dl-load.c:654)
==447374==    by 0x400974D: _dl_map_object (dl-load.c:2071)
==447374==    by 0x400DDC0: openaux (dl-deps.c:64)
==447374==    by 0x4F0225F: _dl_catch_exception (dl-error-skeleton.c:208)
==447374==    by 0x400E138: _dl_map_object_deps (dl-deps.c:248)
==447374==    by 0x4013DAA: dl_open_worker (dl-open.c:571)
==447374==    by 0x4F0225F: _dl_catch_exception (dl-error-skeleton.c:208)
==447374==  Address 0x82e0d19 is 9 bytes inside a block of size 15 alloc'd
==447374==    at 0x483877F: malloc (vg_replace_malloc.c:299)
==447374==    by 0x401C16A: strdup (strdup.c:42)
==447374==    by 0x4008B74: decompose_rpath (dl-load.c:606)
==447374==    by 0x400974D: cache_rpath (dl-load.c:673)
==447374==    by 0x400974D: cache_rpath (dl-load.c:654)
==447374==    by 0x400974D: _dl_map_object (dl-load.c:2071)
==447374==    by 0x400DDC0: openaux (dl-deps.c:64)
==447374==    by 0x4F0225F: _dl_catch_exception (dl-error-skeleton.c:208)
==447374==    by 0x400E138: _dl_map_object_deps (dl-deps.c:248)
==447374==    by 0x4013DAA: dl_open_worker (dl-open.c:571)
==447374==    by 0x4F0225F: _dl_catch_exception (dl-error-skeleton.c:208)
==447374==    by 0x40138F9: _dl_open (dl-open.c:837)
==447374==    by 0x5097257: dlopen_doit (dlopen.c:66)
==447374==    by 0x4F0225F: _dl_catch_exception (dl-error-skeleton.c:208)
==447374== 
==447374== Invalid read of size 8
==447374==    at 0x401D604: strncmp (strcmp.S:175)
==447374==    by 0x400604D: is_dst (dl-load.c:209)
==447374==    by 0x400861E: _dl_dst_substitute (dl-load.c:288)
==447374==    by 0x40088D1: fillin_rpath.isra.0 (dl-load.c:460)
==447374==    by 0x4008BE1: decompose_rpath (dl-load.c:631)
==447374==    by 0x400974D: cache_rpath (dl-load.c:673)
==447374==    by 0x400974D: cache_rpath (dl-load.c:654)
==447374==    by 0x400974D: _dl_map_object (dl-load.c:2071)
==447374==    by 0x400DDC0: openaux (dl-deps.c:64)
==447374==    by 0x4F0225F: _dl_catch_exception (dl-error-skeleton.c:208)
==447374==    by 0x400E138: _dl_map_object_deps (dl-deps.c:248)
==447374==    by 0x4013DAA: dl_open_worker (dl-open.c:571)
==447374==    by 0x4F0225F: _dl_catch_exception (dl-error-skeleton.c:208)
==447374==    by 0x40138F9: _dl_open (dl-open.c:837)
==447374==  Address 0x82e0d19 is 9 bytes inside a block of size 15 alloc'd
==447374==    at 0x483877F: malloc (vg_replace_malloc.c:299)
==447374==    by 0x401C16A: strdup (strdup.c:42)
==447374==    by 0x4008B74: decompose_rpath (dl-load.c:606)
==447374==    by 0x400974D: cache_rpath (dl-load.c:673)
==447374==    by 0x400974D: cache_rpath (dl-load.c:654)
==447374==    by 0x400974D: _dl_map_object (dl-load.c:2071)
==447374==    by 0x400DDC0: openaux (dl-deps.c:64)
==447374==    by 0x4F0225F: _dl_catch_exception (dl-error-skeleton.c:208)
==447374==    by 0x400E138: _dl_map_object_deps (dl-deps.c:248)
==447374==    by 0x4013DAA: dl_open_worker (dl-open.c:571)
==447374==    by 0x4F0225F: _dl_catch_exception (dl-error-skeleton.c:208)
==447374==    by 0x40138F9: _dl_open (dl-open.c:837)
==447374==    by 0x5097257: dlopen_doit (dlopen.c:66)
==447374==    by 0x4F0225F: _dl_catch_exception (dl-error-skeleton.c:208)
==447374== 
Input initialized.
SDL initialized successfully!
Embedding main...
Embedding graphics...
Embedding sprites...
Embedding input...
Embedding maths...
Embedding text...
Embedding file I/O...
Embedding audio...
Squirrel initialized successfully!
Imported core lib.
Imported actors lib.

================

brux
tux.brx
Working directory: /home/kelvin/Code/tuxadv
==447374== Mismatched free() / delete / delete []
==447374==    at 0x483A08B: operator delete(void*, unsigned long) (vg_replace_malloc.c:585)
==447374==    by 0x11D9B8: main (main.cpp:66)
==447374==  Address 0x82b2ac0 is 0 bytes inside a block of size 25 alloc'd
==447374==    at 0x483AD7B: realloc (vg_replace_malloc.c:826)
==447374==    by 0x4EB9EFF: getcwd (getcwd.c:84)
==447374==    by 0x11D981: main (main.cpp:64)
==447374== 
-f
Running tux.brx...
Running src/util.nut...
Running src/text.nut...
Running src/shapes.nut...
Running src/tilemap.nut...
Running src/assets.nut...
libpng warning: iCCP: known incorrect sRGB profile
libpng warning: iCCP: known incorrect sRGB profile
libpng warning: iCCP: known incorrect sRGB profile
Running src/global.nut...
Running src/controls.nut...
Running src/menus.nut...
Running src/gmmain.nut...
Running src/gmplay.nut...
Running src/gmpause.nut...
Running src/physactor.nut...
Running src/tux.nut...
Running src/konqi.nut...
Running src/items.nut...
Running src/effects.nut...
Running src/enemies.nut...
Running src/bosses.nut...
Running src/debug.nut...
Running src/bg.nut...
Running src/trigger.nut...
Running src/water.nut...
Running src/levelend.nut...
Running src/platforms.nut...
Running src/blocks.nut...
Running src/weapons.nut...
Running src/overworld.nut...
Running src/secret.nut...
Running src/npc.nut...
Running src/languagemenu.nut...
Running src/contriblevels.nut...
Running src/save.nut...
Running src/zlist.nut...
Running src/pickchar.nut...
Running src/weather.nut...
Running src/light.nut...
Running src/credits.nut...
Running mods/gfx...
Running mods/test.nut...
Found text.json
Running contrib/oldworld/script.nut...
Loaded The Old World

Loading map: oldworld
Searching for tileset: overworld.png
Adding overworld.png from search path: res/gfx
Searching for tileset: solid.png
Adding solid.png from search path: res/gfx
Searching for tileset: actors.png
Adding actors.png from search path: res/gfx
Running level code...
End level code
Found text.json
Running contrib/frostlands/script.nut...
Loading Frostlands
Loaded Frostlands

Loading map: overworld-F
Searching for tileset: overworld-fl.png
Adding overworld-fl.png from search path: contrib/frostlands/gfx
Searching for tileset: solid.png
Adding solid.png from search path: res/gfx
Searching for tileset: actors.png
Adding actors.png from search path: res/gfx
Running level code...
End level code

Loading map: Fw-3
Searching for tileset: icecavetiles.png
Adding icecavetiles.png from search path: res/gfx
Searching for tileset: actors.png
Adding actors.png from search path: res/gfx
Searching for tileset: tsGrasstop.png
Adding tsGrasstop.png from search path: res/gfx
Searching for tileset: pipetiles.png
Adding pipetiles.png from search path: res/gfx
Searching for tileset: igloo2.png
Adding igloo2.png from search path: contrib/frostlands/gfx
Searching for tileset: FLSnow.png
Adding FLSnow.png from search path: contrib/frostlands/gfx
Searching for tileset: signpost.png
Adding signpost.png from search path: res/gfx
libpng warning: iCCP: known incorrect sRGB profile
Searching for tileset: goal.png
Adding goal.png from search path: res/gfx
Searching for tileset: solid.png
Adding solid.png from search path: res/gfx
Running level code...
End level code

================

Cleaning up all resources...
Cleaning textures...
==447374== Invalid read of size 8
==447374==    at 0x4AE5FFC: ??? (in /usr/lib/x86_64-linux-gnu/libSDL2-2.0.so.0.14.0)
==447374==    by 0x11BF13: xyDeleteImage(unsigned int) (graphics.cpp:225)
==447374==    by 0x11E037: xyEnd() (main.cpp:206)
==447374==    by 0x11DAF9: main (main.cpp:89)
==447374==  Address 0x270fbfc0 is 0 bytes inside a block of size 44 free'd
==447374==    at 0x48399AB: free (vg_replace_malloc.c:530)
==447374==    by 0x7F262DF: ??? (in /usr/lib/x86_64-linux-gnu/libGLX_mesa.so.0.0.0)
==447374==    by 0x7F27E8B: ??? (in /usr/lib/x86_64-linux-gnu/libGLX_mesa.so.0.0.0)
==447374==    by 0x870766A: ??? (in /usr/lib/x86_64-linux-gnu/dri/radeonsi_dri.so)
==447374==    by 0x8708F23: ??? (in /usr/lib/x86_64-linux-gnu/dri/radeonsi_dri.so)
==447374==    by 0x871462B: ??? (in /usr/lib/x86_64-linux-gnu/dri/radeonsi_dri.so)
==447374==    by 0x8714C47: ??? (in /usr/lib/x86_64-linux-gnu/dri/radeonsi_dri.so)
==447374==    by 0x87355E4: ??? (in /usr/lib/x86_64-linux-gnu/dri/radeonsi_dri.so)
==447374==    by 0x873A500: ??? (in /usr/lib/x86_64-linux-gnu/dri/radeonsi_dri.so)
==447374==    by 0x4AEACD0: ??? (in /usr/lib/x86_64-linux-gnu/libSDL2-2.0.so.0.14.0)
==447374==    by 0x4ADF6E0: ??? (in /usr/lib/x86_64-linux-gnu/libSDL2-2.0.so.0.14.0)
==447374==    by 0x4ADFD1C: ??? (in /usr/lib/x86_64-linux-gnu/libSDL2-2.0.so.0.14.0)
==447374==  Block was alloc'd at
==447374==    at 0x483877F: malloc (vg_replace_malloc.c:299)
==447374==    by 0x5CE3812: ??? (in /usr/lib/x86_64-linux-gnu/libxcb.so.1.1.0)
==447374==    by 0x5CE4187: xcb_poll_for_special_event (in /usr/lib/x86_64-linux-gnu/libxcb.so.1.1.0)
==447374==    by 0x7F262EE: ??? (in /usr/lib/x86_64-linux-gnu/libGLX_mesa.so.0.0.0)
==447374==    by 0x7F27E8B: ??? (in /usr/lib/x86_64-linux-gnu/libGLX_mesa.so.0.0.0)
==447374==    by 0x870766A: ??? (in /usr/lib/x86_64-linux-gnu/dri/radeonsi_dri.so)
==447374==    by 0x8708F23: ??? (in /usr/lib/x86_64-linux-gnu/dri/radeonsi_dri.so)
==447374==    by 0x871462B: ??? (in /usr/lib/x86_64-linux-gnu/dri/radeonsi_dri.so)
==447374==    by 0x8714C47: ??? (in /usr/lib/x86_64-linux-gnu/dri/radeonsi_dri.so)
==447374==    by 0x87355E4: ??? (in /usr/lib/x86_64-linux-gnu/dri/radeonsi_dri.so)
==447374==    by 0x873A500: ??? (in /usr/lib/x86_64-linux-gnu/dri/radeonsi_dri.so)
==447374==    by 0x4AEACD0: ??? (in /usr/lib/x86_64-linux-gnu/libSDL2-2.0.so.0.14.0)
==447374== 
Cleaning sprites...
Cleaning sounds...
Cleaning music...
Finished cleanup.
Closing Squirrel...
Collected 0 junk obects.
Closing SDL...
System closed successfully!
==447374== 
==447374== HEAP SUMMARY:
==447374==     in use at exit: 368,425 bytes in 4,259 blocks
==447374==   total heap usage: 616,170 allocs, 611,911 frees, 108,554,648 bytes allocated
==447374== 
==447374== 1,135 (864 direct, 271 indirect) bytes in 9 blocks are definitely lost in loss record 2,737 of 2,777
==447374==    at 0x4838DEF: operator new(unsigned long) (vg_replace_malloc.c:334)
==447374==    by 0x114106: sqNewSprite(SQVM*) (binds.cpp:525)
==447374==    by 0x14BE7F: SQVM::CallNative(SQNativeClosure*, long long, long long, SQObjectPtr&, int, bool&, bool&) (in /home/kelvin/Code/brux/rte/bin/brux)
==447374==    by 0x150303: SQVM::Execute(SQObjectPtr&, long long, long long, SQObjectPtr&, unsigned long long, SQVM::ExecutionType) (in /home/kelvin/Code/brux/rte/bin/brux)
==447374==    by 0x151BE2: SQVM::Call(SQObjectPtr&, long long, long long, SQObjectPtr&, unsigned long long) (in /home/kelvin/Code/brux/rte/bin/brux)
==447374==    by 0x12AEF7: sq_call (in /home/kelvin/Code/brux/rte/bin/brux)
==447374==    by 0x15A776: sqstd_dofile (in /home/kelvin/Code/brux/rte/bin/brux)
==447374==    by 0x11DA90: main (main.cpp:82)
==447374== 
==447374== 4,302 (144 direct, 4,158 indirect) bytes in 2 blocks are definitely lost in loss record 2,767 of 2,777
==447374==    at 0x4838DEF: operator new(unsigned long) (vg_replace_malloc.c:334)
==447374==    by 0x115BA9: sqNewFont(SQVM*) (binds.cpp:1139)
==447374==    by 0x14BE7F: SQVM::CallNative(SQNativeClosure*, long long, long long, SQObjectPtr&, int, bool&, bool&) (in /home/kelvin/Code/brux/rte/bin/brux)
==447374==    by 0x150303: SQVM::Execute(SQObjectPtr&, long long, long long, SQObjectPtr&, unsigned long long, SQVM::ExecutionType) (in /home/kelvin/Code/brux/rte/bin/brux)
==447374==    by 0x151BE2: SQVM::Call(SQObjectPtr&, long long, long long, SQObjectPtr&, unsigned long long) (in /home/kelvin/Code/brux/rte/bin/brux)
==447374==    by 0x12AEF7: sq_call (in /home/kelvin/Code/brux/rte/bin/brux)
==447374==    by 0x15A776: sqstd_dofile (in /home/kelvin/Code/brux/rte/bin/brux)
==447374==    by 0x1132AF: sqDoNut(SQVM*) (binds.cpp:176)
==447374==    by 0x14BE7F: SQVM::CallNative(SQNativeClosure*, long long, long long, SQObjectPtr&, int, bool&, bool&) (in /home/kelvin/Code/brux/rte/bin/brux)
==447374==    by 0x150303: SQVM::Execute(SQObjectPtr&, long long, long long, SQObjectPtr&, unsigned long long, SQVM::ExecutionType) (in /home/kelvin/Code/brux/rte/bin/brux)
==447374==    by 0x151BE2: SQVM::Call(SQObjectPtr&, long long, long long, SQObjectPtr&, unsigned long long) (in /home/kelvin/Code/brux/rte/bin/brux)
==447374==    by 0x12AEF7: sq_call (in /home/kelvin/Code/brux/rte/bin/brux)
==447374== 
==447374== 16,301 (12,576 direct, 3,725 indirect) bytes in 131 blocks are definitely lost in loss record 2,775 of 2,777
==447374==    at 0x4838DEF: operator new(unsigned long) (vg_replace_malloc.c:334)
==447374==    by 0x114106: sqNewSprite(SQVM*) (binds.cpp:525)
==447374==    by 0x14BE7F: SQVM::CallNative(SQNativeClosure*, long long, long long, SQObjectPtr&, int, bool&, bool&) (in /home/kelvin/Code/brux/rte/bin/brux)
==447374==    by 0x150303: SQVM::Execute(SQObjectPtr&, long long, long long, SQObjectPtr&, unsigned long long, SQVM::ExecutionType) (in /home/kelvin/Code/brux/rte/bin/brux)
==447374==    by 0x151BE2: SQVM::Call(SQObjectPtr&, long long, long long, SQObjectPtr&, unsigned long long) (in /home/kelvin/Code/brux/rte/bin/brux)
==447374==    by 0x12AEF7: sq_call (in /home/kelvin/Code/brux/rte/bin/brux)
==447374==    by 0x15A776: sqstd_dofile (in /home/kelvin/Code/brux/rte/bin/brux)
==447374==    by 0x1132AF: sqDoNut(SQVM*) (binds.cpp:176)
==447374==    by 0x14BE7F: SQVM::CallNative(SQNativeClosure*, long long, long long, SQObjectPtr&, int, bool&, bool&) (in /home/kelvin/Code/brux/rte/bin/brux)
==447374==    by 0x150303: SQVM::Execute(SQObjectPtr&, long long, long long, SQObjectPtr&, unsigned long long, SQVM::ExecutionType) (in /home/kelvin/Code/brux/rte/bin/brux)
==447374==    by 0x151BE2: SQVM::Call(SQObjectPtr&, long long, long long, SQObjectPtr&, unsigned long long) (in /home/kelvin/Code/brux/rte/bin/brux)
==447374==    by 0x12AEF7: sq_call (in /home/kelvin/Code/brux/rte/bin/brux)
==447374== 
==447374== LEAK SUMMARY:
==447374==    definitely lost: 13,584 bytes in 142 blocks
==447374==    indirectly lost: 8,154 bytes in 142 blocks
==447374==      possibly lost: 0 bytes in 0 blocks
==447374==    still reachable: 344,671 bytes in 3,954 blocks
==447374==         suppressed: 0 bytes in 0 blocks
==447374== Reachable blocks (those to which a pointer was found) are not shown.
==447374== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==447374== 
==447374== For counts of detected and suppressed errors, rerun with: -v
==447374== ERROR SUMMARY: 9 errors from 7 contexts (suppressed: 0 from 0)
ghost commented 2 years ago

So I followed the chain of functions that get called starting from sqDeleteSprite, and fortunately the issue in question is pretty easy to fix. That said, probably best to start from the top, as I'm unsure of where the mix-up stems from.

SQInteger sqDeleteSprite(HSQUIRRELVM v) {
    SQInteger i;

    sq_getinteger(v, 2, &i);

    if(i >= vcSprites.size()) return 0;
        //pretty straight-forward, delete is used to invoke the destructor for the sprite
    if(vcSprites[i] != 0) delete vcSprites[i];

    return 0;
};

...then since there is a destructor defined for the xySprite class, it jumps to here:

xySprite::~xySprite() {
    //again pretty simple, the memory is made inaccessible by removing the pointer to the object
    if(numero == vcSprites.size() - 1) vcSprites.pop_back(); else vcSprites[numero] = 0;
    xyDeleteImage(tex);
};

...and now we wind up where the problem is at, in xyDeleteImage:

void xyDeleteImage(Uint32 tex) {
  if(tex > vcSprites.size()) return;

  //the associated vcTexture is deleted here
  SDL_DestroyTexture(vcTextures[tex]);

 //but then it's the vcSprites vector that gets modified, again
  if(tex < vcSprites.size() - 1) vcSprites[tex] = 0;
  else vcSprites.pop_back();
};

So yeah, seems like the problem was just that the wrong pointers are getting removed after deleting the texture. It's a little surprising that this didn't creep up sooner, but based on where this issue stemmed from, my guess is that it only worked out by chance, as typically sprites were only deleted when they were also stored at the end of vector. Which combined with textures also typically being in 1-to-1 relation with the sprites, would cause the initial if-statement in xyDeleteImage to trigger.

That last part being said, I would be mindful of just outright deleting the texture in response to the sprite being deleted. Primarily because of the newSpriteFT bind, which opens the door for one texture to be used in multiple sprites. So if any of one of those sprites were to get deleted, then the texture would be corrupted for the others. You would most likely need some form of associative container (i.e. a map) to track which textures are used by which sprites, and then use that to determine when it's actually safe to delete them.

ghost commented 1 year ago

Should be fixed by https://github.com/KelvinShadewing/brux-gdk/pull/44, thus making this closeable