The pattern of foo = realloc(foo, size); causes a memory leak if realloc fails because realloc does not free foo in case of failure, but foo will be NULL after a failure, so there is no way to access or free the original foo.
Fix unchecked [mc]alloc return values as well.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-memleak/flash.h =================================================================== --- flashrom-memleak/flash.h (Revision 725) +++ flashrom-memleak/flash.h (Arbeitskopie) @@ -323,7 +323,7 @@ void print_supported_chipsets(void); void print_supported_boards(void); void print_supported_pcidevs(struct pcidev_status *devs); -void print_wiki_tables(void); +int print_wiki_tables(void);
/* board_enable.c */ void w836xx_ext_enter(uint16_t port); Index: flashrom-memleak/ft2232_spi.c =================================================================== --- flashrom-memleak/ft2232_spi.c (Revision 725) +++ flashrom-memleak/ft2232_spi.c (Arbeitskopie) @@ -193,14 +193,17 @@ { struct ftdi_context *ftdic = &ftdic_context; static unsigned char *buf = NULL; + unsigned char *newbuf = NULL; int i, ret = 0;
if (writecnt > 65536 || readcnt > 65536) return SPI_INVALID_LENGTH;
- buf = realloc(buf, writecnt + readcnt + 100); - if (!buf) { + newbuf = realloc(buf, writecnt + readcnt + 100); + if (!newbuf) { fprintf(stderr, "Out of memory!\n"); + free(buf); + buf = NULL; exit(1); }
Index: flashrom-memleak/flashrom.c =================================================================== --- flashrom-memleak/flashrom.c (Revision 725) +++ flashrom-memleak/flashrom.c (Arbeitskopie) @@ -282,9 +282,12 @@
char *strcat_realloc(char *dest, const char *src) { - dest = realloc(dest, strlen(dest) + strlen(src) + 1); - if (!dest) + char *newdest = realloc(dest, strlen(dest) + strlen(src) + 1); + if (!newdest) { + free(dest); + fprintf(stderr, "Could not allocate memory!\n"); return NULL; + } strcat(dest, src); return dest; } @@ -459,6 +462,10 @@ printf("Error: No filename specified.\n"); return 1; } + if (!buf) { + fprintf(stderr, "Could not allocate memory!\n"); + exit(1); + } if ((image = fopen(filename, "w")) == NULL) { perror(filename); exit(1); @@ -829,7 +836,8 @@
#if CONFIG_PRINT_WIKI == 1 if (list_supported_wiki) { - print_wiki_tables(); + if (print_wiki_tables()) + exit(1); exit(0); } #endif @@ -944,6 +952,10 @@
size = flash->total_size * 1024; buf = (uint8_t *) calloc(size, sizeof(char)); + if (!buf) { + fprintf(stderr, "Could not allocate memory!\n"); + exit(1); + }
if (erase_it) { if (flash->tested & TEST_BAD_ERASE) { Index: flashrom-memleak/print_wiki.c =================================================================== --- flashrom-memleak/print_wiki.c (Revision 725) +++ flashrom-memleak/print_wiki.c (Arbeitskopie) @@ -353,7 +353,7 @@ printf("\n|}\n\n|}\n"); }
-static void wiki_helper(const char *heading, const char *status, +static int wiki_helper(const char *heading, const char *status, int cols, const struct board_info boards[]) { int i, j, k, c, boardcount = 0, color = 1, num_notes = 0; @@ -362,6 +362,10 @@ char *notes = calloc(1, 1); char tmp[900 + 1];
+ if (!notes) { + fprintf(stderr, "Could not allocate memory!\n"); + return 1; + } for (b = boards; b->vendor != NULL; b++) boardcount++;
@@ -388,6 +392,10 @@ snprintf((char *)&tmp, 900, "<sup>%d</sup> %s<br />\n", 1 + num_notes++, boards_notes[c].note); notes = strcat_realloc(notes, (char *)&tmp); + if (!notes) { + return 1; + } + } else { printf("\n"); } @@ -404,6 +412,8 @@ if (num_notes > 0) printf("\n<small>\n%s</small>\n", notes); free(notes); + + return 0; }
static void wiki_helper2(const char *heading, int cols) @@ -447,16 +457,21 @@ printf("\n|}\n\n|}\n"); }
-void print_supported_boards_wiki(void) +int print_supported_boards_wiki(void) { printf("%s", board_intro); - wiki_helper("Known good (worked out of the box)", "OK", 3, boards_ok); + if (wiki_helper("Known good (worked out of the box)", "OK", 3, boards_ok)) + return 1; wiki_helper2("Known good (with write-enable code in flashrom)", 3); - wiki_helper("Not supported (yet)", "No", 3, boards_bad); + if (wiki_helper("Not supported (yet)", "No", 3, boards_bad)) + return 1;
printf("%s", laptop_intro); - wiki_helper("Known good (worked out of the box)", "OK", 1, laptops_ok); - wiki_helper("Not supported (yet)", "No", 1, laptops_bad); + if (wiki_helper("Known good (worked out of the box)", "OK", 1, laptops_ok)) + return 1; + if (wiki_helper("Not supported (yet)", "No", 1, laptops_bad)) + return 1; + return 0; }
void print_supported_chips_wiki(void) @@ -524,14 +539,15 @@ } }
-void print_wiki_tables(void) +int print_wiki_tables(void) { time_t t = time(NULL);
printf(wiki_header, ctime(&t), flashrom_version); print_supported_chips_wiki(); print_supported_chipsets_wiki(); - print_supported_boards_wiki(); + if (print_supported_boards_wiki()) + return 1; printf("%s", programmer_section); #if NIC3COM_SUPPORT == 1 print_supported_pcidevs_wiki(nics_3com); @@ -543,5 +559,6 @@ print_supported_pcidevs_wiki(satas_sii); #endif printf("\n|}\n"); + return 0; }
Index: flashrom-memleak/print.c =================================================================== --- flashrom-memleak/print.c (Revision 725) +++ flashrom-memleak/print.c (Arbeitskopie) @@ -31,25 +31,48 @@ char *flashbuses_to_text(enum chipbustype bustype) { char *ret = calloc(1, 1); + if (!ret) { + fprintf(stderr, "Could not allocate memory!\n"); + return NULL; + } if (bustype == CHIP_BUSTYPE_UNKNOWN) { ret = strcat_realloc(ret, "Unknown,"); + if (!ret) + return NULL; /* * FIXME: Once all chipsets and flash chips have been updated, NONSPI * will cease to exist and should be eliminated here as well. */ } else if (bustype == CHIP_BUSTYPE_NONSPI) { ret = strcat_realloc(ret, "Non-SPI,"); + if (!ret) + return NULL; } else { - if (bustype & CHIP_BUSTYPE_PARALLEL) + if (bustype & CHIP_BUSTYPE_PARALLEL) { ret = strcat_realloc(ret, "Parallel,"); - if (bustype & CHIP_BUSTYPE_LPC) + if (!ret) + return NULL; + } + if (bustype & CHIP_BUSTYPE_LPC) { ret = strcat_realloc(ret, "LPC,"); - if (bustype & CHIP_BUSTYPE_FWH) + if (!ret) + return NULL; + } + if (bustype & CHIP_BUSTYPE_FWH) { ret = strcat_realloc(ret, "FWH,"); - if (bustype & CHIP_BUSTYPE_SPI) + if (!ret) + return NULL; + } + if (bustype & CHIP_BUSTYPE_SPI) { ret = strcat_realloc(ret, "SPI,"); - if (bustype == CHIP_BUSTYPE_NONE) + if (!ret) + return NULL; + } + if (bustype == CHIP_BUSTYPE_NONE) { ret = strcat_realloc(ret, "None,"); + if (!ret) + return NULL; + } } /* Kill last comma. */ ret[strlen(ret) - 1] = '\0';
On Wed, Sep 16, 2009 at 02:48:18PM +0200, Carl-Daniel Hailfinger wrote:
The pattern of foo = realloc(foo, size); causes a memory leak if realloc fails because realloc does not free foo in case of failure, but foo will be NULL after a failure, so there is no way to access or free the original foo.
Fix unchecked [mc]alloc return values as well.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Looks good and is build-tested by me. But please see below.
Acked-by: Uwe Hermann uwe@hermann-uwe.de
Index: flashrom-memleak/flashrom.c
--- flashrom-memleak/flashrom.c (Revision 725) +++ flashrom-memleak/flashrom.c (Arbeitskopie) @@ -282,9 +282,12 @@
char *strcat_realloc(char *dest, const char *src) {
- dest = realloc(dest, strlen(dest) + strlen(src) + 1);
- if (!dest)
- char *newdest = realloc(dest, strlen(dest) + strlen(src) + 1);
- if (!newdest) {
free(dest);fprintf(stderr, "Could not allocate memory!\n");- } strcat(dest, src); return dest;
This looks strange, newdest is the new string but we still use dest in the strcat below? Is that correct?
Uwe.
On 18.09.2009 15:33, Uwe Hermann wrote:
On Wed, Sep 16, 2009 at 02:48:18PM +0200, Carl-Daniel Hailfinger wrote:
The pattern of foo = realloc(foo, size); causes a memory leak if realloc fails because realloc does not free foo in case of failure, but foo will be NULL after a failure, so there is no way to access or free the original foo.
Fix unchecked [mc]alloc return values as well.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Looks good and is build-tested by me. But please see below.
Acked-by: Uwe Hermann uwe@hermann-uwe.de
Thanks, I'll resend with fixes.
- char *newdest = realloc(dest, strlen(dest) + strlen(src) + 1); strcat(dest, src);
This looks strange, newdest is the new string but we still use dest in the strcat below? Is that correct?
Oops. This is a real bug. Also present in other places. Will fix.
Regards, Carl-Daniel
Next try. Uwe, could you please check if I fixed all new bugs? Thanks.
The pattern of foo = realloc(foo, size); causes a memory leak if realloc fails because realloc does not free foo in case of failure, but foo will be NULL after a failure, so there is no way to access or free the original foo.
Fix unchecked [mc]alloc return values as well.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-memleak/flash.h =================================================================== --- flashrom-memleak/flash.h (Revision 727) +++ flashrom-memleak/flash.h (Arbeitskopie) @@ -323,7 +323,7 @@ void print_supported_chipsets(void); void print_supported_boards(void); void print_supported_pcidevs(struct pcidev_status *devs); -void print_wiki_tables(void); +int print_wiki_tables(void);
/* board_enable.c */ void w836xx_ext_enter(uint16_t port); Index: flashrom-memleak/ft2232_spi.c =================================================================== --- flashrom-memleak/ft2232_spi.c (Revision 727) +++ flashrom-memleak/ft2232_spi.c (Arbeitskopie) @@ -193,19 +193,21 @@ { struct ftdi_context *ftdic = &ftdic_context; static unsigned char *buf = NULL; - int i, ret = 0; + unsigned char *newbuf = NULL; + int i = 0, ret = 0;
if (writecnt > 65536 || readcnt > 65536) return SPI_INVALID_LENGTH;
- buf = realloc(buf, writecnt + readcnt + 100); - if (!buf) { + newbuf = realloc(buf, writecnt + readcnt + 100); + if (!newbuf) { fprintf(stderr, "Out of memory!\n"); + free(buf); + buf = NULL; exit(1); } + buf = newbuf;
- i = 0; - /* minimize USB transfers by packing as many commands * as possible together. if we're not expecting to * read, we can assert CS, write, and deassert CS all Index: flashrom-memleak/flashrom.c =================================================================== --- flashrom-memleak/flashrom.c (Revision 727) +++ flashrom-memleak/flashrom.c (Arbeitskopie) @@ -282,9 +282,13 @@
char *strcat_realloc(char *dest, const char *src) { - dest = realloc(dest, strlen(dest) + strlen(src) + 1); - if (!dest) + char *newdest = realloc(dest, strlen(dest) + strlen(src) + 1); + if (!newdest) { + free(dest); + fprintf(stderr, "Could not allocate memory!\n"); return NULL; + } + dest = newdest; strcat(dest, src); return dest; } @@ -459,6 +463,10 @@ printf("Error: No filename specified.\n"); return 1; } + if (!buf) { + fprintf(stderr, "Could not allocate memory!\n"); + exit(1); + } if ((image = fopen(filename, "w")) == NULL) { perror(filename); exit(1); @@ -829,7 +837,8 @@
#if PRINT_WIKI_SUPPORT == 1 if (list_supported_wiki) { - print_wiki_tables(); + if (print_wiki_tables()) + exit(1); exit(0); } #endif @@ -944,6 +953,10 @@
size = flash->total_size * 1024; buf = (uint8_t *) calloc(size, sizeof(char)); + if (!buf) { + fprintf(stderr, "Could not allocate memory!\n"); + exit(1); + }
if (erase_it) { if (flash->tested & TEST_BAD_ERASE) { Index: flashrom-memleak/print_wiki.c =================================================================== --- flashrom-memleak/print_wiki.c (Revision 727) +++ flashrom-memleak/print_wiki.c (Arbeitskopie) @@ -353,7 +353,7 @@ printf("\n|}\n\n|}\n"); }
-static void wiki_helper(const char *heading, const char *status, +static int wiki_helper(const char *heading, const char *status, int cols, const struct board_info boards[]) { int i, j, k, c, boardcount = 0, color = 1, num_notes = 0; @@ -362,6 +362,10 @@ char *notes = calloc(1, 1); char tmp[900 + 1];
+ if (!notes) { + fprintf(stderr, "Could not allocate memory!\n"); + return 1; + } for (b = boards; b->vendor != NULL; b++) boardcount++;
@@ -388,6 +392,10 @@ snprintf((char *)&tmp, 900, "<sup>%d</sup> %s<br />\n", 1 + num_notes++, boards_notes[c].note); notes = strcat_realloc(notes, (char *)&tmp); + if (!notes) { + return 1; + } + } else { printf("\n"); } @@ -404,6 +412,8 @@ if (num_notes > 0) printf("\n<small>\n%s</small>\n", notes); free(notes); + + return 0; }
static void wiki_helper2(const char *heading, int cols) @@ -447,16 +457,21 @@ printf("\n|}\n\n|}\n"); }
-void print_supported_boards_wiki(void) +int print_supported_boards_wiki(void) { printf("%s", board_intro); - wiki_helper("Known good (worked out of the box)", "OK", 3, boards_ok); + if (wiki_helper("Known good (worked out of the box)", "OK", 3, boards_ok)) + return 1; wiki_helper2("Known good (with write-enable code in flashrom)", 3); - wiki_helper("Not supported (yet)", "No", 3, boards_bad); + if (wiki_helper("Not supported (yet)", "No", 3, boards_bad)) + return 1;
printf("%s", laptop_intro); - wiki_helper("Known good (worked out of the box)", "OK", 1, laptops_ok); - wiki_helper("Not supported (yet)", "No", 1, laptops_bad); + if (wiki_helper("Known good (worked out of the box)", "OK", 1, laptops_ok)) + return 1; + if (wiki_helper("Not supported (yet)", "No", 1, laptops_bad)) + return 1; + return 0; }
void print_supported_chips_wiki(void) @@ -524,14 +539,15 @@ } }
-void print_wiki_tables(void) +int print_wiki_tables(void) { time_t t = time(NULL);
printf(wiki_header, ctime(&t), flashrom_version); print_supported_chips_wiki(); print_supported_chipsets_wiki(); - print_supported_boards_wiki(); + if (print_supported_boards_wiki()) + return 1; printf("%s", programmer_section); #if NIC3COM_SUPPORT == 1 print_supported_pcidevs_wiki(nics_3com); @@ -543,5 +559,6 @@ print_supported_pcidevs_wiki(satas_sii); #endif printf("\n|}\n"); + return 0; }
Index: flashrom-memleak/print.c =================================================================== --- flashrom-memleak/print.c (Revision 727) +++ flashrom-memleak/print.c (Arbeitskopie) @@ -31,25 +31,48 @@ char *flashbuses_to_text(enum chipbustype bustype) { char *ret = calloc(1, 1); + if (!ret) { + fprintf(stderr, "Could not allocate memory!\n"); + return NULL; + } if (bustype == CHIP_BUSTYPE_UNKNOWN) { ret = strcat_realloc(ret, "Unknown,"); + if (!ret) + return NULL; /* * FIXME: Once all chipsets and flash chips have been updated, NONSPI * will cease to exist and should be eliminated here as well. */ } else if (bustype == CHIP_BUSTYPE_NONSPI) { ret = strcat_realloc(ret, "Non-SPI,"); + if (!ret) + return NULL; } else { - if (bustype & CHIP_BUSTYPE_PARALLEL) + if (bustype & CHIP_BUSTYPE_PARALLEL) { ret = strcat_realloc(ret, "Parallel,"); - if (bustype & CHIP_BUSTYPE_LPC) + if (!ret) + return NULL; + } + if (bustype & CHIP_BUSTYPE_LPC) { ret = strcat_realloc(ret, "LPC,"); - if (bustype & CHIP_BUSTYPE_FWH) + if (!ret) + return NULL; + } + if (bustype & CHIP_BUSTYPE_FWH) { ret = strcat_realloc(ret, "FWH,"); - if (bustype & CHIP_BUSTYPE_SPI) + if (!ret) + return NULL; + } + if (bustype & CHIP_BUSTYPE_SPI) { ret = strcat_realloc(ret, "SPI,"); - if (bustype == CHIP_BUSTYPE_NONE) + if (!ret) + return NULL; + } + if (bustype == CHIP_BUSTYPE_NONE) { ret = strcat_realloc(ret, "None,"); + if (!ret) + return NULL; + } } /* Kill last comma. */ ret[strlen(ret) - 1] = '\0';
OK, if you're going to do this:
if (x) { message exit }
all over the place, why not do this instead:
void *xalloc(size_t amount, char *msg) { void *ret = alloc(amount); if (! ret) { fprintf(stderr, "%s: Alloc of %d bytes failed\n", msg, amount); exit(1); } }
same for realloc.
Why? because: 1. you're repeating the same code over and over 2. you're not telling people where you failed, or how much you tried to allocate this kind of fatal error should be informative
just a comment
ron
On 18/09/2009, ron minnich rminnich@gmail.com wrote:
OK, if you're going to do this:
if (x) { message exit }
all over the place, why not do this instead:
void *xalloc(size_t amount, char *msg) { void *ret = alloc(amount); if (! ret) { fprintf(stderr, "%s: Alloc of %d bytes failed\n", msg, amount); exit(1); } }
same for realloc.
Why? because:
- you're repeating the same code over and over
- you're not telling people where you failed, or how much you tried to
allocate this kind of fatal error should be informative
just a comment
if xalloc will be done as macro then its possible to use __LINE__ and __FILE__ values to show from where xalloc was called
#declare xalloc(a,b) xalloc(a,b,__FILE__, __LINE__)
or similar construction
just my 1e-30 of comments
best regards Maciej
On Sat, Sep 19, 2009 at 4:13 AM, Maciej Pijanka maciej.pijanka@gmail.com wrote:
if xalloc will be done as macro then its possible to use __LINE__ and __FILE__ values to show from where xalloc was called
#declare xalloc(a,b) xalloc(a,b,__FILE__, __LINE__)
that's fine too. The main thing is that when an allocate fails you want as much information as possible, esp. if you are going to report a bug :-)
Also, sometimes, seeing that it tried to allocate 981340981732871234 bytes can help :-)
ron
-
Carl-Daniel Hailfinger -
Maciej Pijanka -
ron minnich -
Uwe Hermann