[parted-devel] parted failures with DEBUG turned *off*

[David Cantrell]

ACK.  BTW, thanks again for diving through all of this code.

On Tue, 2007-06-05 at 12:19 +0200, Jim Meyering wrote:
> I spent the first part of yesterday debugging the ext2-resize failure.
> As part of that, I turned off DEBUG and was surprised to see new failures
> in the label checks.  At least for label types "amiga" and "bsd", the
> implementation required that freshly-allocated memory be filled with all
> "1" bits, as was guaranteed by the default setting of
> 
> #define DEBUG 1
> 
> When I turned that off, bsd.c would read/write uninitialized memory, and
> rdb.c(amiga) would do more of the same and produce partition tables that
> it would then fail to recognize.
> 
> Here's the fix for the bsd problems.
> I'll send the rdb/amiga ones separately, and once all tests pass
> without malloc-initialization-to-all-1's, I'll remove that, too.
> 
> The bsd read-uninit bug was at bsd.c:341 (bsd_write), with this test:
> 
> 	if (!bsd_specific->boot_code [0])
> 		_probe_and_add_boot_code (disk);
> 
> that first byte was never initialized.
> So, I figured, that'll be easy.  Just initialize it.
> Wrong.  That wasn't enough, since then a part of that same 512-byte
> buffer (starting at offset 340) would be used uninitialized by
> a write syscall.
> 
> FYI, the first failure was demonstrated like this:
> 
> dev=f
> N=1M
> dd if=/dev/zero of=$dev bs=$N count=1 && valgrind ./parted -s $dev mklabel bsd
> 
> Here's the first one:
> 
> ==20087== Conditional jump or move depends on uninitialised value(s)
> ==20087==    at 0x4429EE: bsd_write (bsd.c:341)
> ==20087==    by 0x411AD3: ped_disk_commit_to_dev (disk.c:485)
> ==20087==    by 0x411B19: ped_disk_commit (disk.c:508)
> ==20087==    by 0x403A12: do_mklabel (parted.c:622)
> ==20087==    by 0x402AF7: command_run (command.c:139)
> ==20087==    by 0x40B00A: non_interactive_mode (ui.c:1530)
> ==20087==    by 0x407A8B: main (parted.c:2479)
> 
> and even after initializing only that first byte, here's what I got:
> 
> ==25692== Syscall param write(buf) points to uninitialised byte(s)
> ==25692==    at 0x54874D0: __write_nocancel (in /usr/lib/debug/libc-2.5.so)
> ==25692==    by 0x41A15C: linux_write (linux.c:1599)
> ==25692==    by 0x40D9CA: ped_device_write (device.c:369)
> ==25692==    by 0x442B1E: bsd_write (bsd.c:368)
> ==25692==    by 0x411AD3: ped_disk_commit_to_dev (disk.c:485)
> ==25692==    by 0x411B19: ped_disk_commit (disk.c:508)
> ==25692==    by 0x403A12: do_mklabel (parted.c:622)
> ==25692==    by 0x402AF7: command_run (command.c:139)
> ==25692==    by 0x40B00A: non_interactive_mode (ui.c:1530)
> ==25692==    by 0x407A8B: main (parted.c:2479)
> ==25692==  Address 0x59E9C01 is 340 bytes inside a block of size 512 alloc'd
> ==25692==    at 0x4A1EC7C: memalign (vg_replace_malloc.c:332)
> ==25692==    by 0x4A1ECD5: posix_memalign (vg_replace_malloc.c:425)
> ==25692==    by 0x41A11A: linux_write (linux.c:1594)
> ==25692==    by 0x40D9CA: ped_device_write (device.c:369)
> ==25692==    by 0x442B1E: bsd_write (bsd.c:368)
> ==25692==    by 0x411AD3: ped_disk_commit_to_dev (disk.c:485)
> ==25692==    by 0x411B19: ped_disk_commit (disk.c:508)
> ==25692==    by 0x403A12: do_mklabel (parted.c:622)
> ==25692==    by 0x402AF7: command_run (command.c:139)
> ==25692==    by 0x40B00A: non_interactive_mode (ui.c:1530)
> ==25692==    by 0x407A8B: main (parted.c:2479)
> 
> Here's the patch:
> 
> diff --git a/libparted/labels/bsd.c b/libparted/labels/bsd.c
> index 747a9c5..5963242 100644
> --- a/libparted/labels/bsd.c
> +++ b/libparted/labels/bsd.c
> @@ -182,9 +182,14 @@ bsd_alloc (const PedDevice* dev)
>  	disk->disk_specific = bsd_specific = ped_malloc (sizeof (BSDDiskData));
>  	if (!bsd_specific)
>  		goto error_free_disk;
> +        /* Initialize the first byte to zero, so that the code in bsd_write
> +           knows to call _probe_and_add_boot_code.  Initializing all of the
> +           remaining buffer is a little wasteful, but the alternative is to
> +           figure out why a block at offset 340 would otherwise be used
> +           uninitialized.  */
> +	memset(bsd_specific->boot_code, 0, sizeof (bsd_specific->boot_code));
>  
>  	label = (BSDRawLabel*) (bsd_specific->boot_code + BSD_LABEL_OFFSET);
> -	memset(label, 0, sizeof(BSDRawLabel));
>  
>  	label->d_magic = PED_CPU_TO_LE32 (BSD_DISKMAGIC);
>  	label->d_type = PED_CPU_TO_LE16 (BSD_DTYPE_SCSI);
> 
> 
> Any objection should be accompanied with a better patch
> and comments explaining "why"  :)
> 
> _______________________________________________
> parted-devel mailing list
> parted-devel at lists.alioth.debian.org
> http://lists.alioth.debian.org/mailman/listinfo/parted-devel
-- 
David Cantrell <dcantrell at redhat.com>
Red Hat / Westford, MA
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://lists.alioth.debian.org/pipermail/parted-devel/attachments/20070605/f850aa39/attachment.pgp