Fixed bug 2318 - h->cm_map resource getting leak in read_xcf_header function
authorSam Lantinga <slouken@libsdl.org>
Mon, 11 Sep 2017 21:21:30 -0700
changeset 499d3e819a08733
parent 498 af656a0a0fb5
child 500 28967769bc2e
Fixed bug 2318 - h->cm_map resource getting leak in read_xcf_header function

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;
}
IMG_xcf.c
     1.1 --- a/IMG_xcf.c	Sat Sep 09 18:57:05 2017 -0700
     1.2 +++ b/IMG_xcf.c	Mon Sep 11 21:21:30 2017 -0700
     1.3 @@ -298,6 +298,9 @@
     1.4    xcf_prop prop;
     1.5  
     1.6    h = (xcf_header *) SDL_malloc (sizeof (xcf_header));
     1.7 +  if (!h) {
     1.8 +    return NULL;
     1.9 +  }
    1.10    SDL_RWread (src, h->sign, 14, 1);
    1.11    h->width       = SDL_ReadBE32 (src);
    1.12    h->height      = SDL_ReadBE32 (src);
    1.13 @@ -316,11 +319,22 @@
    1.14        h->compr = (xcf_compr_type)prop.data.compression;
    1.15      else if (prop.id == PROP_COLORMAP) {
    1.16        // unused var: int i;
    1.17 +      Uint32 cm_num;
    1.18 +      unsigned char *cm_map;
    1.19  
    1.20 -      h->cm_num = prop.data.colormap.num;
    1.21 -      h->cm_map = (unsigned char *) SDL_malloc (sizeof (unsigned char) * 3 * h->cm_num);
    1.22 -      SDL_memcpy (h->cm_map, prop.data.colormap.cmap, 3*sizeof (char)*h->cm_num);
    1.23 +      cm_num = prop.data.colormap.num;
    1.24 +      cm_map = (unsigned char *) SDL_realloc(h->cm_map, sizeof (unsigned char) * 3 * cm_num);
    1.25 +      if (cm_map) {
    1.26 +        h->cm_num = cm_num;
    1.27 +        h->cm_map = cm_map;
    1.28 +        SDL_memcpy (h->cm_map, prop.data.colormap.cmap, 3*sizeof (char)*h->cm_num);
    1.29 +      }
    1.30        SDL_free (prop.data.colormap.cmap);
    1.31 +
    1.32 +      if (!cm_map) {
    1.33 +        free_xcf_header(h);
    1.34 +        return NULL;
    1.35 +      }
    1.36      }
    1.37    } while (prop.id != PROP_END);
    1.38  
    1.39 @@ -544,149 +558,143 @@
    1.40    SDL_FillRect (surf, NULL, c);
    1.41  }
    1.42  
    1.43 -static int do_layer_surface (SDL_Surface * surface, SDL_RWops * src, xcf_header * head, xcf_layer * layer, load_tile_type load_tile) {
    1.44 -  xcf_hierarchy * hierarchy;
    1.45 -  xcf_level     * level;
    1.46 -  unsigned char * tile;
    1.47 -  Uint8  * p8;
    1.48 -  Uint16 * p16;
    1.49 -  Uint32 * p;
    1.50 -  int i, j;
    1.51 -  Uint32 x, y, tx, ty, ox, oy;
    1.52 -  Uint32 *row;
    1.53 +static int 
    1.54 +do_layer_surface(SDL_Surface * surface, SDL_RWops * src, xcf_header * head, xcf_layer * layer, load_tile_type load_tile)
    1.55 +{
    1.56 +    xcf_hierarchy  *hierarchy;
    1.57 +    xcf_level      *level;
    1.58 +    unsigned char  *tile;
    1.59 +    Uint8          *p8;
    1.60 +    Uint16         *p16;
    1.61 +    Uint32         *p;
    1.62 +    int            i, j;
    1.63 +    Uint32         x, y, tx, ty, ox, oy;
    1.64 +    Uint32         *row;
    1.65  
    1.66 -  SDL_RWseek (src, layer->hierarchy_file_offset, RW_SEEK_SET);
    1.67 -  hierarchy = read_xcf_hierarchy (src);
    1.68 +    SDL_RWseek(src, layer->hierarchy_file_offset, RW_SEEK_SET);
    1.69 +    hierarchy = read_xcf_hierarchy(src);
    1.70  
    1.71 -  level = NULL;
    1.72 -  for (i = 0; hierarchy->level_file_offsets [i]; i++) {
    1.73 -    SDL_RWseek (src, hierarchy->level_file_offsets [i], RW_SEEK_SET);
    1.74 -    level = read_xcf_level (src);
    1.75 +    level = NULL;
    1.76 +    for (i = 0; hierarchy->level_file_offsets[i]; i++) {
    1.77 +        SDL_RWseek(src, hierarchy->level_file_offsets[i], RW_SEEK_SET);
    1.78 +        level = read_xcf_level(src);
    1.79  
    1.80 -    ty = tx = 0;
    1.81 -    for (j = 0; level->tile_file_offsets [j]; j++) {
    1.82 -      SDL_RWseek (src, level->tile_file_offsets [j], RW_SEEK_SET);
    1.83 -      ox = tx+64 > level->width ? level->width % 64 : 64;
    1.84 -      oy = ty+64 > level->height ? level->height % 64 : 64;
    1.85 +        ty = tx = 0;
    1.86 +        for (j = 0; level->tile_file_offsets[j]; j++) {
    1.87 +            SDL_RWseek(src, level->tile_file_offsets[j], RW_SEEK_SET);
    1.88 +            ox = tx + 64 > level->width ? level->width % 64 : 64;
    1.89 +            oy = ty + 64 > level->height ? level->height % 64 : 64;
    1.90  
    1.91 -      if (level->tile_file_offsets [j+1]) {
    1.92 -    tile = load_tile
    1.93 -      (src,
    1.94 -       level->tile_file_offsets [j+1] - level->tile_file_offsets [j],
    1.95 -       hierarchy->bpp,
    1.96 -       ox, oy);
    1.97 -      }
    1.98 -      else {
    1.99 -    tile = load_tile
   1.100 -      (src,
   1.101 -       ox*oy*6,
   1.102 -       hierarchy->bpp,
   1.103 -       ox, oy);
   1.104 -      }
   1.105 +            if (level->tile_file_offsets[j + 1]) {
   1.106 +                tile = load_tile(src, level->tile_file_offsets[j + 1] - level->tile_file_offsets[j], hierarchy->bpp, ox, oy);
   1.107 +            } else {
   1.108 +                tile = load_tile(src, ox * oy * 6, hierarchy->bpp, ox, oy);
   1.109 +            }
   1.110  
   1.111 -      p8  = tile;
   1.112 -      p16 = (Uint16 *) p8;
   1.113 -      p   = (Uint32 *) p8;
   1.114 -      for (y=ty; y < ty+oy; y++) {
   1.115 -    row = (Uint32 *)((Uint8 *)surface->pixels + y*surface->pitch + tx*4);
   1.116 -    switch (hierarchy->bpp) {
   1.117 -    case 4:
   1.118 -      for (x=tx; x < tx+ox; x++)
   1.119 -        *row++ = Swap32 (*p++);
   1.120 -      break;
   1.121 -    case 3:
   1.122 -      for (x=tx; x < tx+ox; x++) {
   1.123 -        *row = 0xFF000000;
   1.124 -        *row |= ((Uint32) *(p8++) << 16);
   1.125 -        *row |= ((Uint32) *(p8++) << 8);
   1.126 -        *row |= ((Uint32) *(p8++) << 0);
   1.127 -        row++;
   1.128 -      }
   1.129 -      break;
   1.130 -    case 2: // Indexed/Greyscale + Alpha
   1.131 -      switch (head->image_type) {
   1.132 -      case IMAGE_INDEXED:
   1.133 -        for (x=tx; x < tx+ox; x++) {
   1.134 -          *row =  ((Uint32) (head->cm_map [*p8*3])     << 16);
   1.135 -          *row |= ((Uint32) (head->cm_map [*p8*3+1])   << 8);
   1.136 -          *row |= ((Uint32) (head->cm_map [*p8++*3+2]) << 0);
   1.137 -          *row |= ((Uint32) *p8++ << 24);;
   1.138 -          row++;
   1.139 +            p8 = tile;
   1.140 +            p16 = (Uint16 *) p8;
   1.141 +            p = (Uint32 *) p8;
   1.142 +            for (y = ty; y < ty + oy; y++) {
   1.143 +                row = (Uint32 *) ((Uint8 *) surface->pixels + y * surface->pitch + tx * 4);
   1.144 +                switch (hierarchy->bpp) {
   1.145 +                case 4:
   1.146 +                    for (x = tx; x < tx + ox; x++)
   1.147 +                        *row++ = Swap32(*p++);
   1.148 +                    break;
   1.149 +                case 3:
   1.150 +                    for (x = tx; x < tx + ox; x++) {
   1.151 +                        *row = 0xFF000000;
   1.152 +                        *row |= ((Uint32)*p8++ << 16);
   1.153 +                        *row |= ((Uint32)*p8++ << 8);
   1.154 +                        *row |= ((Uint32)*p8++ << 0);
   1.155 +                        row++;
   1.156 +                    }
   1.157 +                    break;
   1.158 +                case 2:
   1.159 +                    /* Indexed / Greyscale + Alpha */
   1.160 +                    switch (head->image_type) {
   1.161 +                    case IMAGE_INDEXED:
   1.162 +                        for (x = tx; x < tx + ox; x++) {
   1.163 +                            *row = ((Uint32)(head->cm_map[*p8 * 3]) << 16);
   1.164 +                            *row |= ((Uint32)(head->cm_map[*p8 * 3 + 1]) << 8);
   1.165 +                            *row |= ((Uint32)(head->cm_map[*p8++ * 3 + 2]) << 0);
   1.166 +                            *row |= ((Uint32)*p8++ << 24);
   1.167 +                            row++;
   1.168 +                        }
   1.169 +                        break;
   1.170 +                    case IMAGE_GREYSCALE:
   1.171 +                        for (x = tx; x < tx + ox; x++) {
   1.172 +                            *row = ((Uint32)*p8 << 16);
   1.173 +                            *row |= ((Uint32)*p8 << 8);
   1.174 +                            *row |= ((Uint32)*p8++ << 0);
   1.175 +                            *row |= ((Uint32)*p8++ << 24);
   1.176 +                            row++;
   1.177 +                        }
   1.178 +                        break;
   1.179 +                    default:
   1.180 +                        fprintf(stderr, "Unknown Gimp image type (%d)\n", head->image_type);
   1.181 +                        if (hierarchy) {
   1.182 +                            if (hierarchy->level_file_offsets)
   1.183 +                                SDL_free(hierarchy->level_file_offsets);
   1.184 +
   1.185 +                            free_xcf_hierarchy(hierarchy);
   1.186 +                        }
   1.187 +                        if (level)
   1.188 +                            free_xcf_level(level);
   1.189 +                        return 1;
   1.190 +                    }
   1.191 +                    break;
   1.192 +                case 1:
   1.193 +                    /* Indexed / Greyscale */
   1.194 +					switch (head->image_type) {
   1.195 +                    case IMAGE_INDEXED:
   1.196 +                        for (x = tx; x < tx + ox; x++) {
   1.197 +                            *row++ = 0xFF000000
   1.198 +                                | ((Uint32)(head->cm_map[*p8 * 3]) << 16)
   1.199 +                                | ((Uint32)(head->cm_map[*p8 * 3 + 1]) << 8)
   1.200 +                                | ((Uint32)(head->cm_map[*p8 * 3 + 2]) << 0);
   1.201 +                            p8++;
   1.202 +                        }
   1.203 +                        break;
   1.204 +                    case IMAGE_GREYSCALE:
   1.205 +                        for (x = tx; x < tx + ox; x++) {
   1.206 +                            *row++ = 0xFF000000
   1.207 +                                | (((Uint32)(*p8)) << 16)
   1.208 +                                | (((Uint32)(*p8)) << 8)
   1.209 +                                | (((Uint32)(*p8)) << 0);
   1.210 +                            ++p8;
   1.211 +                        }
   1.212 +                        break;
   1.213 +                    default:
   1.214 +                        fprintf(stderr, "Unknown Gimp image type (%d)\n", head->image_type);
   1.215 +                        if (tile)
   1.216 +                            free_xcf_tile(tile);
   1.217 +                        if (level)
   1.218 +                            free_xcf_level(level);
   1.219 +                        if (hierarchy)
   1.220 +                            free_xcf_hierarchy(hierarchy);
   1.221 +                        return 1;
   1.222 +                    }
   1.223 +                    break;
   1.224 +                }
   1.225 +            }
   1.226 +            free_xcf_tile(tile);
   1.227 +
   1.228 +            tx += 64;
   1.229 +            if (tx >= level->width) {
   1.230 +                tx = 0;
   1.231 +                ty += 64;
   1.232 +            }
   1.233 +            if (ty >= level->height) {
   1.234 +                break;
   1.235 +            }
   1.236          }
   1.237 -        break;
   1.238 -      case IMAGE_GREYSCALE:
   1.239 -        for (x=tx; x < tx+ox; x++) {
   1.240 -          *row = ((Uint32) *p8 << 16);
   1.241 -          *row |= ((Uint32) *p8 << 8);
   1.242 -          *row |= ((Uint32) *p8++ << 0);
   1.243 -          *row |= ((Uint32) *p8++ << 24);;
   1.244 -          row++;
   1.245 -        }
   1.246 -        break;
   1.247 -      default:
   1.248 -        fprintf (stderr, "Unknown Gimp image type (%d)\n", head->image_type);
   1.249 -        if (hierarchy)
   1.250 -        {
   1.251 -          if (hierarchy->level_file_offsets)
   1.252 -            SDL_free(hierarchy->level_file_offsets);
   1.253 -         
   1.254 -          free_xcf_hierarchy(hierarchy);
   1.255 -        }
   1.256 -        if (level)     
   1.257 -          free_xcf_level (level);
   1.258 -        return 1;
   1.259 -      }
   1.260 -      break;
   1.261 -    case 1: // Indexed/Greyscale
   1.262 -      switch (head->image_type) {
   1.263 -      case IMAGE_INDEXED:
   1.264 -        for (x = tx; x < tx+ox; x++) {
   1.265 -          *row++ = 0xFF000000
   1.266 -        | ((Uint32) (head->cm_map [*p8*3]) << 16)
   1.267 -        | ((Uint32) (head->cm_map [*p8*3+1]) << 8)
   1.268 -        | ((Uint32) (head->cm_map [*p8*3+2]) << 0);
   1.269 -          p8++;
   1.270 -        }
   1.271 -        break;
   1.272 -      case IMAGE_GREYSCALE:
   1.273 -        for (x=tx; x < tx+ox; x++) {
   1.274 -          *row++ = 0xFF000000
   1.275 -        | (((Uint32) (*p8)) << 16)
   1.276 -        | (((Uint32) (*p8)) << 8)
   1.277 -        | (((Uint32) (*p8)) << 0);
   1.278 -            ++p8;
   1.279 -        }
   1.280 -        break;
   1.281 -      default:
   1.282 -        fprintf (stderr, "Unknown Gimp image type (%d)\n", head->image_type);
   1.283 -        if (tile)
   1.284 -          free_xcf_tile (tile);
   1.285 -        if (level)
   1.286 -          free_xcf_level (level);
   1.287 -        if (hierarchy)
   1.288 -          free_xcf_hierarchy (hierarchy);
   1.289 -        return 1;
   1.290 -      }
   1.291 -      break;
   1.292 +        free_xcf_level(level);
   1.293      }
   1.294 -      }
   1.295 -      tx += 64;
   1.296 -      if (tx >= level->width) {
   1.297 -    tx = 0;
   1.298 -    ty += 64;
   1.299 -      }
   1.300 -      if (ty >= level->height) {
   1.301 -    break;
   1.302 -      }
   1.303  
   1.304 -      free_xcf_tile (tile);
   1.305 -    }
   1.306 -    free_xcf_level (level);
   1.307 -  }
   1.308 +    free_xcf_hierarchy(hierarchy);
   1.309  
   1.310 -  free_xcf_hierarchy (hierarchy);
   1.311 -
   1.312 -  return 0;
   1.313 +    return 0;
   1.314  }
   1.315  
   1.316  SDL_Surface *IMG_LoadXCF_RW(SDL_RWops *src)
   1.317 @@ -702,7 +710,7 @@
   1.318  
   1.319    unsigned char * (* load_tile) (SDL_RWops *, Uint32, int, int, int);
   1.320  
   1.321 -  if ( !src ) {
   1.322 +  if (!src) {
   1.323      /* The error message has been set in SDL_RWFromFile */
   1.324      return NULL;
   1.325    }
   1.326 @@ -711,7 +719,10 @@
   1.327    /* Initialize the data we will clean up when we're done */
   1.328    surface = NULL;
   1.329  
   1.330 -  head = read_xcf_header (src);
   1.331 +  head = read_xcf_header(src);
   1.332 +  if (!head) {
   1.333 +    return NULL;
   1.334 +  }
   1.335  
   1.336    switch (head->compr) {
   1.337    case COMPR_NONE: