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

[Jim Meyering]

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"  :)