Re: [PATCH]: Save boot record before writing to the dest_drive

[Mikko Rantalainen]

kashyap garimella wrote at 2009-09-12:
> Greetings!
> 
> I have added the following new features:
> 
> 1) when grub-setup runs, it automatically stores the areas of (mbr + embed)
> region, which are overwritten, into the file (in the following specified
> format) in root directory.
> 
> 2) grub-setup can restore the stored mbr in the following format back to
> destination drive:
> [...]
> 
> PROBLEMS:
> A warning:
> util/i386/pc/grub-setup.c:93: warning: ‘backup_img’ may be used
> uninitialized in this function
> When I remove the line (496 in the patched one):   free(backup_img);
> the warning goes off.
> I could not understand how to resolve it. I need some help

I looked at the patch and it seems that the problem could be caused by
following code:

> +  if (!backup_fp)
> +  {
> +    strcpy(grub_errmsg,"Unable to create the backup file");
> +    goto unable_to_save;
> +  }
> +
> +  /* 8 bytes for embed_region.start, another 8 for core_size */
> +  int backup_img_size =  core_size + GRUB_DISK_SECTOR_SIZE + 8  + 8 + 1 /* 
> seperation byte 0xff */ ;
> +  backup_img = (char *) xmalloc (backup_img_size);
...
> +save_finish:  
> +  fclose (backup_fp);
> +  free(backup_path);
> +  free(backup_img);

Notice that you're using "goto" to skip initialization of backup_img and
you blindly free()ing it in "save_finish".

I think a nice way to fix this (if you want to use "goto") is to use
multiple clean up labels. Like this:

+save_cleanup_backup_img:
+  free(backup_img);
+save_cleanup_backup_fp:
+  fclose (backup_fp);
+save_cleanup_backup_path:
+  free(backup_path);

Note that you should release the resources in reverse order compared to
the order of acquiring the resources. If you need to bail out after you
have acquired the backup_fp but have not yet acquired the backup_img,
use "goto save_cleanup_backup_fp".

-- 
Mikko

signature.asc
Description: OpenPGP digital signature