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 > [email protected] > http://lists.alioth.debian.org/mailman/listinfo/parted-devel -- David Cantrell <[EMAIL PROTECTED]> Red Hat / Westford, MA
signature.asc
Description: This is a digitally signed message part
_______________________________________________ parted-devel mailing list [email protected] http://lists.alioth.debian.org/mailman/listinfo/parted-devel
