TLN_LoadSpriteset: Valgrind shows that memory is lost

[thacoon]

When testing my projects unit tests with valgrind for memory losses, I noticed that using TLN_LoadSpriteset resulted in bytes getting lost. I reproduced this behaviour with an example program, see below. I also added the Valgrind output I got below:

gcc -lTilengine Tutorial.c && valgrind --track-origins=yes --leak-check=full -q ./a.out 
Tilengine v2.4.0 64-bit built Nov  8 2019 19:43:19
==16072== 1,044 bytes in 1 blocks are definitely lost in loss record 2 of 2
==16072==    at 0x483AB65: calloc (vg_replace_malloc.c:762)
==16072==    by 0x4892515: CreateBaseObject (in /usr/lib/libTilengine.so)
==16072==    by 0x489CA89: TLN_CreatePalette (in /usr/lib/libTilengine.so)
==16072==    by 0x489867A: ??? (in /usr/lib/libTilengine.so)
==16072==    by 0x48986FF: TLN_LoadBitmap (in /usr/lib/libTilengine.so)
==16072==    by 0x488E915: TLN_LoadSpriteset (in /usr/lib/libTilengine.so)
==16072==    by 0x109205: main (in /home/username/Downloads/Tilengine/samples/a.out)
==16072==

#include <Tilengine.h>

#define HRES	424
#define VRES	240

int main(int argc, char* argv[])
{
    TLN_Init(HRES, VRES, 1, 8, 8);

    TLN_Spriteset player;
    TLN_Sequence idle, skip;

    /* load assets */
    TLN_SetLoadPath("assets/forest");
    player = TLN_LoadSpriteset("player");

    /* create sprite sequences */
    idle = TLN_CreateSpriteSequence(NULL, player, "player-idle-", 9, 6);
    skip = TLN_CreateSpriteSequence(NULL, player, "player-skip-", 8, 6);

    TLN_ConfigSprite(0, player, 0);
    TLN_SetSpriteAnimation(0, 0, idle, 0);

    /* free sprites/sequences */
    TLN_DeleteSequence(idle);
    TLN_DeleteSequence(skip);
    TLN_DeleteSpriteset(player);

    TLN_Deinit();

    return 0;
}

[ghost]

Hi all

@megamarc
The 1044 byte memory leak can be seen in the log:

Tilengine v2.4.0 64-bit built Nov 17 2019 19:51:47                                                                                     
Tilengine: bitmap created at 0x5555555cebb0, 23720 size                                                                                
Tilengine: palette created at 0x5555555ce6a0, 1044 size                                                                                
Tilengine: spriteset created at 0x5555555caf10, 312 size                                                                               
Tilengine: sequence created at 0x5555555ceac0, 144 size                                                                                
Tilengine: sequence created at 0x5555555cb050, 136 size                                                                                
Tilengine: sequence 0x5555555ceac0 deleted                                                                                             
Tilengine: sequence 0x5555555cb050 deleted                                                                                             
Tilengine: bitmap 0x5555555cebb0 deleted                                                                                               
Tilengine: spriteset 0x5555555caf10 deleted 

The palette is never deleted.
The function TLN_DeleteSpriteset call to DeleteBaseObject (spriteset->bitmap) but this does not delete the palette, as the function does TLN_DeleteBitmap(need attention here for #50)

Fixed that I found another leak in the TLN_Deinit function would be missing release engine:
Something like that, I'm not sure:

void TLN_Deinit(void)
{
	if (engine != NULL)
		if (TLN_DeleteContext(engine))
			free(engine);

}

Now, valgrind:

==4536== 
==4536== HEAP SUMMARY:
==4536==     in use at exit: 0 bytes in 0 blocks
==4536==   total heap usage: 36 allocs, 36 frees, 547,947 bytes allocated
==4536== 
==4536== All heap blocks were freed -- no leaks are possible

Regards.

[ghost]

Hi
Today I found some more.
simplexml.c:

Tilengine/src/simplexml.c

Line 332 in ea888ae

free(parser->vbNextToken);

It should be replaced by:
destroySimpleXmlValueBuffer(parser->vbNextToken);
This fixes the memory leak.

New, Invalid read of size:

==3075== Invalid read of size 2                                                                  
==3075==    at 0x483CEE0: memcpy@GLIBC_2.2.5 (vg_replace_strmem.c:1034)                  
==3075==    by 0x4858F0C: TLN_CreateTileset (Tileset.c:101)                                      
==3075==    by 0x4850BA1: TLN_LoadTileset (LoadTileset.c:230)  

The problem is here:

Tilengine/src/Tileset.c

Line 101 in ea888ae

memcpy (tileset->attributes, attributes, size_attributes);

I think the size of size_attributes is incorrect since it is calculated after num++
Maybe it should be like this:

	size_attributes = sizeof(TLN_TileAttributes) * numtiles;
	numtiles++;

It would be equal to the previous calculation in:

Tilengine/src/LoadTileset.c

Line 73 in ea888ae

const int size_attribs = tilecount * sizeof(TLN_TileAttributes);

This fixes the invalid read size.

There is another memory leak in CloneBaseObject.
This function set dst->owner = false
Then somewhare:

	if (ObjectOwner (spriteset))

This conditional does not allow the cloned object to be released, at least I am thinking so.

[megamarc]

Hi guys!
Thanks for your extensive reviews, I'll check all of them. The simplexml.c module is not mine so I don't know its internal logic, but all the others are mine.

[megamarc]

Tilengine uses a shallow object copy model. Only the initially created object owns references to their sibling objects. Cloned objects inherit the reference to the siblings from the original one, no siblings are cloned. That's why only objects created with CreateBaseObject() have the owner property set to true so only them have the right to destroy their siblings. Cloned objects with CloneBaseObject() don't have ownership of the siblings and cannot be destroyed recursively.

[megamarc]

The value of size_attribs after numtiles++ is correct. This is done to include tiles with an id value of 0, that also count. In Tiled Editor the index of a tile is calculated substracting the tile index minus the id of the first valid tile in the tileset, that is 1. This difference between 0 and 1 based index requires incrementing by one the total number of tiles.

[megamarc]

TLN_DeleteContext() is broken. It should be deleting the received context reference, no the global engine. This is a leftover from when it was only an instance. It also lacks a closing free(context) at the end. I'll fix it.

[megamarc]

Memory leak in TLN_DeleteSpriteset():
The owned bitmap is being destroyed with DeleteBaseObject() where it should be done with TLN_DeleteBitmap(). DeleteBaseObject() should only be used on the self object once their siblings have been properly destroyed using their appropiate TLN_DeleteXXX() destructors. It shouldn't be used to destroy siblings, as it cannot follow recursive siblings. Same happens to TLN_DeleteBitmap(), it is improperly destroying its palette, but this hasn't noticeable effect because the palette doesn't contain further siblings. But must be fixed too.