Skip to content

Commit

Permalink
Fixed bug 2318 - h->cm_map resource getting leak in read_xcf_header f…
Browse files Browse the repository at this point in the history 
…unction

Nitz

h->cm_map resource getting leak in static xcf_header * read_xcf_header (SDL_RWops * src) function at the following location:

do {
    xcf_read_property (src, &prop);
    if (prop.id == PROP_COMPRESSION)
      h->compr = (xcf_compr_type)prop.data.compression;
    else if (prop.id == PROP_COLORMAP) {
      // unused var: int i;

      h->cm_num = prop.data.colormap.num;
      h->cm_map = (unsigned char *) SDL_malloc (sizeof (unsigned char) * 3 * h->cm_num);
      SDL_memcpy (h->cm_map, prop.data.colormap.cmap, 3*sizeof (char)*h->cm_num);
      SDL_free (prop.data.colormap.cmap);
    }
  } while (prop.id != PROP_END);

Here Overwriting "h->cm_map" in "h->cm_map = (unsigned char *)malloc(3U * h->cm_num)" leaks the storage that "h->cm_map" points to.

Resource "h->cm_map" is not freed or pointed-to in function "memcpy(void * restrict, void const * restrict, size_t)".

So there is need to check the logic or we can add a patch to avoid this kind of issue.

Sylvain

valgrind reports this memory leak:

==22032==    by 0x4EF0134: SDL_malloc_REAL (SDL_malloc.c:36)
==22032==    by 0x4E7E413: SDL_malloc (SDL_dynapi_procs.h:406)
==22032==    by 0x51EF463: load_xcf_tile_rle (IMG_xcf.c:474)
==22032==    by 0x51EF99A: do_layer_surface (IMG_xcf.c:580)
==22032==    by 0x51EEEED: IMG_LoadXCF_RW (IMG_xcf.c:761)
==22032==    by 0x51DDA66: IMG_LoadTyped_RW (IMG.c:198)
==22032==    by 0x51DD8F2: IMG_Load (IMG.c:139)

That seem to be fixed by doing:

--- a/IMG_xcf.c	Sat Sep 09 18:57:05 2017 -0700
+++ b/IMG_xcf.c	Mon Sep 11 21:43:09 2017 +0200
@@ -676,6 +676,7 @@
     ty += 64;
       }
       if (ty >= level->height) {
+    free_xcf_tile (tile);
     break;
       }
  • Loading branch information
@slouken
slouken committed Sep 12, 2017
1 parent d48a477 commit 20c7fd7
Showing 1 changed file with 156 additions and 145 deletions.
301 changes: 156 additions & 145 deletions IMG_xcf.c
View file
Expand Up @@ -298,6 +298,9 @@ static xcf_header * read_xcf_header (SDL_RWops * src) {
xcf_prop prop;

h = (xcf_header *) SDL_malloc (sizeof (xcf_header));
if (!h) {
return NULL;
}
SDL_RWread (src, h->sign, 14, 1);
h->width = SDL_ReadBE32 (src);
h->height = SDL_ReadBE32 (src);
Expand All @@ -316,11 +319,22 @@ static xcf_header * read_xcf_header (SDL_RWops * src) {
h->compr = (xcf_compr_type)prop.data.compression;
else if (prop.id == PROP_COLORMAP) {
// unused var: int i;

h->cm_num = prop.data.colormap.num;
h->cm_map = (unsigned char *) SDL_malloc (sizeof (unsigned char) * 3 * h->cm_num);
SDL_memcpy (h->cm_map, prop.data.colormap.cmap, 3*sizeof (char)*h->cm_num);
Uint32 cm_num;
unsigned char *cm_map;

cm_num = prop.data.colormap.num;
cm_map = (unsigned char *) SDL_realloc(h->cm_map, sizeof (unsigned char) * 3 * cm_num);
if (cm_map) {
h->cm_num = cm_num;
h->cm_map = cm_map;
SDL_memcpy (h->cm_map, prop.data.colormap.cmap, 3*sizeof (char)*h->cm_num);
}
SDL_free (prop.data.colormap.cmap);

if (!cm_map) {
free_xcf_header(h);
return NULL;
}
}
} while (prop.id != PROP_END);

Expand Down Expand Up @@ -544,149 +558,143 @@ static void create_channel_surface (SDL_Surface * surf, xcf_image_type itype, Ui
SDL_FillRect (surf, NULL, c);
}

static int do_layer_surface (SDL_Surface * surface, SDL_RWops * src, xcf_header * head, xcf_layer * layer, load_tile_type load_tile) {
xcf_hierarchy * hierarchy;
xcf_level * level;
unsigned char * tile;
Uint8 * p8;
Uint16 * p16;
Uint32 * p;
int i, j;
Uint32 x, y, tx, ty, ox, oy;
Uint32 *row;

SDL_RWseek (src, layer->hierarchy_file_offset, RW_SEEK_SET);
hierarchy = read_xcf_hierarchy (src);

level = NULL;
for (i = 0; hierarchy->level_file_offsets [i]; i++) {
SDL_RWseek (src, hierarchy->level_file_offsets [i], RW_SEEK_SET);
level = read_xcf_level (src);

ty = tx = 0;
for (j = 0; level->tile_file_offsets [j]; j++) {
SDL_RWseek (src, level->tile_file_offsets [j], RW_SEEK_SET);
ox = tx+64 > level->width ? level->width % 64 : 64;
oy = ty+64 > level->height ? level->height % 64 : 64;

if (level->tile_file_offsets [j+1]) {
tile = load_tile
(src,
level->tile_file_offsets [j+1] - level->tile_file_offsets [j],
hierarchy->bpp,
ox, oy);
}
else {
tile = load_tile
(src,
ox*oy*6,
hierarchy->bpp,
ox, oy);
}

p8 = tile;
p16 = (Uint16 *) p8;
p = (Uint32 *) p8;
for (y=ty; y < ty+oy; y++) {
row = (Uint32 *)((Uint8 *)surface->pixels + y*surface->pitch + tx*4);
switch (hierarchy->bpp) {
case 4:
for (x=tx; x < tx+ox; x++)
*row++ = Swap32 (*p++);
break;
case 3:
for (x=tx; x < tx+ox; x++) {
*row = 0xFF000000;
*row |= ((Uint32) *(p8++) << 16);
*row |= ((Uint32) *(p8++) << 8);
*row |= ((Uint32) *(p8++) << 0);
row++;
}
break;
case 2: // Indexed/Greyscale + Alpha
switch (head->image_type) {
case IMAGE_INDEXED:
for (x=tx; x < tx+ox; x++) {
*row = ((Uint32) (head->cm_map [*p8*3]) << 16);
*row |= ((Uint32) (head->cm_map [*p8*3+1]) << 8);
*row |= ((Uint32) (head->cm_map [*p8++*3+2]) << 0);
*row |= ((Uint32) *p8++ << 24);;
row++;
}
break;
case IMAGE_GREYSCALE:
for (x=tx; x < tx+ox; x++) {
*row = ((Uint32) *p8 << 16);
*row |= ((Uint32) *p8 << 8);
*row |= ((Uint32) *p8++ << 0);
*row |= ((Uint32) *p8++ << 24);;
row++;
}
break;
default:
fprintf (stderr, "Unknown Gimp image type (%d)\n", head->image_type);
if (hierarchy)
{
if (hierarchy->level_file_offsets)
SDL_free(hierarchy->level_file_offsets);

free_xcf_hierarchy(hierarchy);
}
if (level)
free_xcf_level (level);
return 1;
}
break;
case 1: // Indexed/Greyscale
switch (head->image_type) {
case IMAGE_INDEXED:
for (x = tx; x < tx+ox; x++) {
*row++ = 0xFF000000
| ((Uint32) (head->cm_map [*p8*3]) << 16)
| ((Uint32) (head->cm_map [*p8*3+1]) << 8)
| ((Uint32) (head->cm_map [*p8*3+2]) << 0);
p8++;
}
break;
case IMAGE_GREYSCALE:
for (x=tx; x < tx+ox; x++) {
*row++ = 0xFF000000
| (((Uint32) (*p8)) << 16)
| (((Uint32) (*p8)) << 8)
| (((Uint32) (*p8)) << 0);
++p8;
static int
do_layer_surface(SDL_Surface * surface, SDL_RWops * src, xcf_header * head, xcf_layer * layer, load_tile_type load_tile)
{
xcf_hierarchy *hierarchy;
xcf_level *level;
unsigned char *tile;
Uint8 *p8;
Uint16 *p16;
Uint32 *p;
int i, j;
Uint32 x, y, tx, ty, ox, oy;
Uint32 *row;

SDL_RWseek(src, layer->hierarchy_file_offset, RW_SEEK_SET);
hierarchy = read_xcf_hierarchy(src);

level = NULL;
for (i = 0; hierarchy->level_file_offsets[i]; i++) {
SDL_RWseek(src, hierarchy->level_file_offsets[i], RW_SEEK_SET);
level = read_xcf_level(src);

ty = tx = 0;
for (j = 0; level->tile_file_offsets[j]; j++) {
SDL_RWseek(src, level->tile_file_offsets[j], RW_SEEK_SET);
ox = tx + 64 > level->width ? level->width % 64 : 64;
oy = ty + 64 > level->height ? level->height % 64 : 64;

if (level->tile_file_offsets[j + 1]) {
tile = load_tile(src, level->tile_file_offsets[j + 1] - level->tile_file_offsets[j], hierarchy->bpp, ox, oy);
} else {
tile = load_tile(src, ox * oy * 6, hierarchy->bpp, ox, oy);
}

p8 = tile;
p16 = (Uint16 *) p8;
p = (Uint32 *) p8;
for (y = ty; y < ty + oy; y++) {
row = (Uint32 *) ((Uint8 *) surface->pixels + y * surface->pitch + tx * 4);
switch (hierarchy->bpp) {
case 4:
for (x = tx; x < tx + ox; x++)
*row++ = Swap32(*p++);
break;
case 3:
for (x = tx; x < tx + ox; x++) {
*row = 0xFF000000;
*row |= ((Uint32)*p8++ << 16);
*row |= ((Uint32)*p8++ << 8);
*row |= ((Uint32)*p8++ << 0);
row++;
}
break;
case 2:
/* Indexed / Greyscale + Alpha */
switch (head->image_type) {
case IMAGE_INDEXED:
for (x = tx; x < tx + ox; x++) {
*row = ((Uint32)(head->cm_map[*p8 * 3]) << 16);
*row |= ((Uint32)(head->cm_map[*p8 * 3 + 1]) << 8);
*row |= ((Uint32)(head->cm_map[*p8++ * 3 + 2]) << 0);
*row |= ((Uint32)*p8++ << 24);
row++;
}
break;
case IMAGE_GREYSCALE:
for (x = tx; x < tx + ox; x++) {
*row = ((Uint32)*p8 << 16);
*row |= ((Uint32)*p8 << 8);
*row |= ((Uint32)*p8++ << 0);
*row |= ((Uint32)*p8++ << 24);
row++;
}
break;
default:
fprintf(stderr, "Unknown Gimp image type (%d)\n", head->image_type);
if (hierarchy) {
if (hierarchy->level_file_offsets)
SDL_free(hierarchy->level_file_offsets);

free_xcf_hierarchy(hierarchy);
}
if (level)
free_xcf_level(level);
return 1;
}
break;
case 1:
/* Indexed / Greyscale */
switch (head->image_type) {
case IMAGE_INDEXED:
for (x = tx; x < tx + ox; x++) {
*row++ = 0xFF000000
| ((Uint32)(head->cm_map[*p8 * 3]) << 16)
| ((Uint32)(head->cm_map[*p8 * 3 + 1]) << 8)
| ((Uint32)(head->cm_map[*p8 * 3 + 2]) << 0);
p8++;
}
break;
case IMAGE_GREYSCALE:
for (x = tx; x < tx + ox; x++) {
*row++ = 0xFF000000
| (((Uint32)(*p8)) << 16)
| (((Uint32)(*p8)) << 8)
| (((Uint32)(*p8)) << 0);
++p8;
}
break;
default:
fprintf(stderr, "Unknown Gimp image type (%d)\n", head->image_type);
if (tile)
free_xcf_tile(tile);
if (level)
free_xcf_level(level);
if (hierarchy)
free_xcf_hierarchy(hierarchy);
return 1;
}
break;
}
}
free_xcf_tile(tile);

tx += 64;
if (tx >= level->width) {
tx = 0;
ty += 64;
}
if (ty >= level->height) {
break;
}
}
break;
default:
fprintf (stderr, "Unknown Gimp image type (%d)\n", head->image_type);
if (tile)
free_xcf_tile (tile);
if (level)
free_xcf_level (level);
if (hierarchy)
free_xcf_hierarchy (hierarchy);
return 1;
}
break;
free_xcf_level(level);
}
}
tx += 64;
if (tx >= level->width) {
tx = 0;
ty += 64;
}
if (ty >= level->height) {
break;
}

free_xcf_tile (tile);
}
free_xcf_level (level);
}
free_xcf_hierarchy(hierarchy);

free_xcf_hierarchy (hierarchy);

return 0;
return 0;
}

SDL_Surface *IMG_LoadXCF_RW(SDL_RWops *src)
Expand All @@ -702,7 +710,7 @@ SDL_Surface *IMG_LoadXCF_RW(SDL_RWops *src)

unsigned char * (* load_tile) (SDL_RWops *, Uint32, int, int, int);

if ( !src ) {
if (!src) {
/* The error message has been set in SDL_RWFromFile */
return NULL;
}
Expand All @@ -711,7 +719,10 @@ SDL_Surface *IMG_LoadXCF_RW(SDL_RWops *src)
/* Initialize the data we will clean up when we're done */
surface = NULL;

head = read_xcf_header (src);
head = read_xcf_header(src);
if (!head) {
return NULL;
}

switch (head->compr) {
case COMPR_NONE:
Expand Down

0 comments on commit 20c7fd7

Please sign in to comment.