New patch to review for coreboot: 154f4a9 cbfstool: Fix leak in cbfs_image struct initialization
Sol Boucher (solb@chromium.org) just uploaded a new patch set to gerrit, which you can find at http://review.coreboot.org/10130
-gerrit
commit 154f4a9689456bde17be275db9c94dba1b5a5d09 Author: Sol Boucher solb@chromium.org Date: Tue May 5 15:40:15 2015 -0700
cbfstool: Fix leak in cbfs_image struct initialization
This patches a memory leak on every struct cbfs_image creation that was introduced by c1d1fd850ee7b8e52bd2ea5064fab68ac0c27098. Since that commit, the CBFS master header has been copied to a separate buffer so that its endianness could be fixed all at once; unfortunately, this buffer was malloc()'d but never free()'d. To address the issue, we replace the structure's struct cbfs_header * with a struct cbfs_header to eliminate the additional allocation.
Change-Id: Ie066c6d4b80ad452b366a2a95092ed45aa55d91f Signed-off-by: Sol Boucher solb@chromium.org --- util/cbfstool/cbfs_image.c | 62 +++++++++++++++++++++------------------------- util/cbfstool/cbfs_image.h | 2 +- util/cbfstool/cbfstool.c | 2 +- 3 files changed, 30 insertions(+), 36 deletions(-)
diff --git a/util/cbfstool/cbfs_image.c b/util/cbfstool/cbfs_image.c index 457a873..3df291a 100644 --- a/util/cbfstool/cbfs_image.c +++ b/util/cbfstool/cbfs_image.c @@ -125,7 +125,7 @@ static int cbfs_fix_legacy_size(struct cbfs_image *image, char *hdr_loc) (char *)entry > (char *)hdr_loc) { WARN("CBFS image was created with old cbfstool with size bug. " "Fixing size in last entry...\n"); - last->len = htonl(ntohl(last->len) - image->header->align); + last->len = htonl(ntohl(last->len) - image->header.align); DEBUG("Last entry has been changed from 0x%x to 0x%x.\n", cbfs_get_entry_addr(image, entry), cbfs_get_entry_addr(image, @@ -215,8 +215,6 @@ int cbfs_image_create(struct cbfs_image *image,
if (buffer_create(&image->buffer, size, "(new)") != 0) return -1; - if ((image->header = malloc(sizeof(*image->header))) == NULL) - return -1; memset(image->buffer.data, CBFS_CONTENT_DEFAULT_VALUE, size);
// Adjust legcay top-aligned address to ROM offset. @@ -252,16 +250,16 @@ int cbfs_image_create(struct cbfs_image *image, header_offset, sizeof(header), size); return -1; } - image->header->magic = CBFS_HEADER_MAGIC; - image->header->version = CBFS_HEADER_VERSION; - image->header->romsize = size; - image->header->bootblocksize = bootblock->size; - image->header->align = align; - image->header->offset = entries_offset; - image->header->architecture = architecture; + image->header.magic = CBFS_HEADER_MAGIC; + image->header.version = CBFS_HEADER_VERSION; + image->header.romsize = size; + image->header.bootblocksize = bootblock->size; + image->header.align = align; + image->header.offset = entries_offset; + image->header.architecture = architecture;
header_loc = (image->buffer.data + header_offset); - cbfs_put_header(header_loc, image->header); + cbfs_put_header(header_loc, &image->header);
// The last 4 byte of the image contain the relative offset from the end // of the image to the master header as a 32-bit signed integer. x86 @@ -319,10 +317,7 @@ int cbfs_image_from_file(struct cbfs_image *image, return -1; }
- if ((image->header = malloc(sizeof(*image->header))) == NULL) - return -1; - - cbfs_get_header(image->header, header_loc); + cbfs_get_header(&image->header, header_loc); cbfs_fix_legacy_size(image, header_loc);
return 0; @@ -339,10 +334,10 @@ int cbfs_copy_instance(struct cbfs_image *image, size_t copy_offset, size_t cbfs_offset, cbfs_end; size_t copy_end = copy_offset + copy_size;
- align = image->header->align; + align = image->header.align;
- cbfs_offset = image->header->offset; - cbfs_end = image->header->romsize; + cbfs_offset = image->header.offset; + cbfs_end = image->header.romsize;
if (copy_end > image->buffer.size) { ERROR("Copy offset out of range: [%zx:%zx)\n", @@ -359,7 +354,7 @@ int cbfs_copy_instance(struct cbfs_image *image, size_t copy_offset,
/* This will work, let's create a copy. */ copy_header = (struct cbfs_header *)(image->buffer.data + copy_offset); - cbfs_put_header(copy_header, image->header); + cbfs_put_header(copy_header, &image->header);
copy_header->bootblocksize = 0; /* Romsize is a misnomer. It's the absolute limit of cbfs content.*/ @@ -414,7 +409,6 @@ int cbfs_image_delete(struct cbfs_image *image) return 0;
buffer_delete(&image->buffer); - image->header = NULL; return 0; }
@@ -433,7 +427,7 @@ static int cbfs_add_entry_at(struct cbfs_image *image, uint32_t header_size = cbfs_calculate_file_header_size(name), min_entry_size = cbfs_calculate_file_header_size(""); uint32_t len, target; - uint32_t align = image->header->align; + uint32_t align = image->header.align;
target = content_offset - header_size; if (target % align) @@ -515,7 +509,7 @@ int cbfs_add_entry(struct cbfs_image *image, struct buffer *buffer,
if (IS_TOP_ALIGNED_ADDRESS(content_offset)) { // legacy cbfstool takes top-aligned address. - uint32_t theromsize = image->header->romsize; + uint32_t theromsize = image->header.romsize; INFO("Converting top-aligned address 0x%x to offset: 0x%x\n", content_offset, content_offset + theromsize); content_offset = theromsize + (int32_t)content_offset; @@ -685,16 +679,16 @@ int cbfs_remove_entry(struct cbfs_image *image, const char *name) int cbfs_print_header_info(struct cbfs_image *image) { char *name = strdup(image->buffer.name); - assert(image && image->header); + assert(image); printf("%s: %zd kB, bootblocksize %d, romsize %d, offset 0x%x\n" "alignment: %d bytes, architecture: %s\n\n", basename(name), image->buffer.size / 1024, - image->header->bootblocksize, - image->header->romsize, - image->header->offset, - image->header->align, - arch_to_string(image->header->architecture)); + image->header.bootblocksize, + image->header.romsize, + image->header.offset, + image->header.align, + arch_to_string(image->header.architecture)); free(name); return 0; } @@ -944,16 +938,16 @@ struct cbfs_header *cbfs_find_header(char *data, size_t size,
struct cbfs_file *cbfs_find_first_entry(struct cbfs_image *image) { - assert(image && image->header); + assert(image); return (struct cbfs_file *)(image->buffer.data + - image->header->offset); + image->header.offset); }
struct cbfs_file *cbfs_find_next_entry(struct cbfs_image *image, struct cbfs_file *entry) { uint32_t addr = cbfs_get_entry_addr(image, entry); - int align = image->header->align; + int align = image->header.align; assert(entry && cbfs_is_valid_entry(image, entry)); addr += ntohl(entry->offset) + ntohl(entry->len); addr = align_up(addr, align); @@ -1017,7 +1011,7 @@ int32_t cbfs_locate_entry(struct cbfs_image *image, const char *name,
/* Default values: allow fitting anywhere in ROM. */ if (!page_size) - page_size = image->header->romsize; + page_size = image->header.romsize; if (!align) align = 1;
@@ -1025,9 +1019,9 @@ int32_t cbfs_locate_entry(struct cbfs_image *image, const char *name, ERROR("Input file size (%d) greater than page size (%d).\n", size, page_size);
- if (page_size % image->header->align) + if (page_size % image->header.align) WARN("%s: Page size (%#x) not aligned with CBFS image (%#x).\n", - __func__, page_size, image->header->align); + __func__, page_size, image->header.align);
/* TODO Old cbfstool always assume input is a stage file (and adding * sizeof(cbfs_stage) for header. We should fix that by adding "-t" diff --git a/util/cbfstool/cbfs_image.h b/util/cbfstool/cbfs_image.h index 1c2b6fd..51d06dc 100644 --- a/util/cbfstool/cbfs_image.h +++ b/util/cbfstool/cbfs_image.h @@ -28,7 +28,7 @@
struct cbfs_image { struct buffer buffer; - struct cbfs_header *header; + struct cbfs_header header; };
/* Given a pointer, serialize the header from host-native byte format diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c index db521d6..024c9cf 100644 --- a/util/cbfstool/cbfstool.c +++ b/util/cbfstool/cbfstool.c @@ -465,7 +465,7 @@ static int cbfs_locate(void) }
if (param.top_aligned) - address = address - image.header->romsize; + address = address - image.header.romsize;
cbfs_image_delete(&image); printf("0x%x\n", address);
-
Sol Boucher