Hi Nisha,
-----Original Message-----
> From: Nisha Parrakat <nisha.parrakat@xxxxxxxx>
>
> Description:
> Fixed memory leaks found based on cppcheck and codesonar run.
>
> Running makedumpfile with valgrind din't show any memory leaks but static
> analysis tools were used to check for possible memory leaks.
>
> Signed-off-by: Nisha Parrakat <Nisha.Parrakat@xxxxxxxx>
Sorry for the delay.
The patch was low priority because it doesn't fix any actual bugs, and
the changes are executed in case of error and makedumpfile will exit
instantly after that, so it doesn't have much effect of saving memory.
And some parts of the patch look unnecessary..
>
> diff -Naur a/dwarf_info.c b/dwarf_info.c
> --- a/dwarf_info.c 2018-07-03 20:52:46.000000000 +0200
> +++ b/dwarf_info.c 2019-01-29 21:17:45.949544759 +0100
> @@ -332,6 +332,8 @@
> dwarf_info.dwfl = dwfl;
> return TRUE;
> err_out:
> + if( dwfl_fd != -1)
> + close(dwfl_fd);
This is unnecessary, because the dwfl_report_offline() function closes
it on success. (and also this will double-close it on failure.)
> if (dwfl)
> dwfl_end(dwfl);
>
> diff -Naur a/erase_info.c b/erase_info.c
> --- a/erase_info.c 2018-07-03 20:52:46.000000000 +0200
> +++ b/erase_info.c 2019-01-29 21:17:45.949544759 +0100
> @@ -599,6 +599,8 @@
> return ce;
>
> err_out:
> + if(ptr && ptr->name) /*free strdup malloced memory*/
> + free( ptr->name );
Did you check the free_config_entry() function below?
> if (ce)
> free_config_entry(ce);
> if (ptr)
> @@ -1030,6 +1032,8 @@
> }
> return config;
> err_out:
> + if (config && config->module_name) /*free strdup memory*/
> + free ( config->module_name );
Did you check the free_config() function below?
> if (config)
> free_config(config);
> return NULL;
>
> diff --git a/makedumpfile.c b/makedumpfile.c
> index 8923538..0f7b90a 100644
> --- a/makedumpfile.c
> +++ b/makedumpfile.c
> @@ -2720,16 +2720,19 @@ copy_vmcoreinfo(off_t offset, unsigned long size)
> if (lseek(info->fd_memory, offset, SEEK_SET) == failed) {
> ERRMSG("Can't seek the dump memory(%s). %s\n",
> info->name_memory, strerror(errno));
> + close(fd);
> return FALSE;
> }
> if (read(info->fd_memory, &buf, size) != size) {
> ERRMSG("Can't read the dump memory(%s). %s\n",
> info->name_memory, strerror(errno));
> + close(fd);
> return FALSE;
> }
> if (write(fd, &buf, size) != size) {
> ERRMSG("Can't write the vmcoreinfo file(%s). %s\n",
> info->name_vmcoreinfo, strerror(errno));
> + close(fd);
> return FALSE;
> }
> if (close(fd) < 0) {
> @@ -3639,6 +3642,7 @@ initialize_bitmap_memory(void)
> if (bmp->buf == NULL) {
> ERRMSG("Can't allocate memory for the bitmap buffer. %s\n",
> strerror(errno));
> + free(bmp);
> return FALSE;
> }
> bmp->fd = info->fd_memory;
> @@ -8486,7 +8490,7 @@ write_eraseinfo(struct cache_data *cd_page, unsigned long *size_out)
> int i, j, obuf_size = 0, ei_size = 0;
> int ret = FALSE;
> unsigned long size_eraseinfo = 0;
> - char *obuf = NULL;
> + char *obuf = NULL; char *obuf_new = NULL;
We don't use this style normally. Instead,
char *obuf = NULL, *obuf_new = NULL;
> char size_str[MAX_SIZE_STR_LEN];
>
> for (i = 1; i < num_erase_info; i++) {
> @@ -8511,12 +8515,14 @@ write_eraseinfo(struct cache_data *cd_page, unsigned long *size_out)
> */
> if (ei_size > obuf_size) {
> obuf_size = ei_size;
> - obuf = realloc(obuf, obuf_size);
> - if (!obuf) {
> + obuf_new = realloc(obuf, obuf_size);
> + if (!obuf_new) {
> ERRMSG("Can't allocate memory for"
> " output buffer\n");
> + free(obuf);
> return FALSE;
"goto out" is better than "free & return" in this function.
> - }
> + }else
> + obuf = obuf_new;
> }
> sprintf(obuf, "erase %s %s", erase_info[i].symbol_expr,
> size_str);
> @@ -9074,12 +9080,14 @@ init_xen_crash_info(void)
> if (lseek(info->fd_memory, offset_xen_crash_info, SEEK_SET) < 0) {
> ERRMSG("Can't seek the dump memory(%s). %s\n",
> info->name_memory, strerror(errno));
> + free(buf);
> return FALSE;
> }
> if (read(info->fd_memory, buf, size_xen_crash_info)
> != size_xen_crash_info) {
> ERRMSG("Can't read the dump memory(%s). %s\n",
> info->name_memory, strerror(errno));
> + free(buf);
> return FALSE;
> }
>
> @@ -10533,7 +10541,7 @@ reassemble_kdump_pages(void)
> struct page_desc pd, pd_zero;
> struct cache_data cd_pd, cd_data;
> struct timespec ts_start;
> - char *data = NULL;
> + char *data = NULL; char* data_new = NULL;
Same as above.
> unsigned long data_buf_size = info->page_size;
>
> if (!prepare_bitmap2_buffer())
> @@ -10663,11 +10671,13 @@ reassemble_kdump_pages(void)
>
> if (SPLITTING_SIZE_EI(i) > data_buf_size) {
> data_buf_size = SPLITTING_SIZE_EI(i);
> - if ((data = realloc(data, data_buf_size)) == NULL) {
> + if ((data_new = realloc(data, data_buf_size)) == NULL) {
> ERRMSG("Can't allocate memory for eraseinfo"
> " data.\n");
> + free(data_new);
This looks wrong.
> goto out;
> - }
> + }else
> + data = data_new;
> }
> if ((fd = open(SPLITTING_DUMPFILE(i), O_RDONLY)) < 0) {
> ERRMSG("Can't open a file(%s). %s\n",
> @@ -11177,12 +11187,14 @@ static int get_sys_kernel_vmcoreinfo(uint64_t *addr, uint64_t *len)
> if (!fgets(line, sizeof(line), fp)) {
> ERRMSG("Cannot parse %s: %s, fgets failed.\n",
> "/sys/kernel/vmcoreinfo", strerror(errno));
> + fclose(fp);
> return FALSE;
> }
> count = sscanf(line, "%Lx %Lx", &temp, &temp2);
> if (count != 2) {
> ERRMSG("Cannot parse %s: %s, sscanf failed.\n",
> "/sys/kernel/vmcoreinfo", strerror(errno));
> + fclose(fp);
> return FALSE;
> }
>
>
Please check the codes and styles around your fixes again.
Thanks,
Kazu
_______________________________________________
kexec mailing list
kexec@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/kexec
