[Parted-commits] GNU Parted Official Repository: Changes to 'master'
Jim Meyering
meyering at alioth.debian.org
Tue Jun 5 15:14:04 UTC 2007
libparted/labels/bsd.c | 7 ++++++-
libparted/labels/rdb.c | 25 ++++++++++++++++++++-----
libparted/libparted.c | 5 ++++-
3 files changed, 30 insertions(+), 7 deletions(-)
New commits:
commit 6628c836bbef4650466bf1c57488aa38bb47a773
Author: Jim Meyering <jim at meyering.net>
Date: Tue Jun 5 16:10:33 2007 +0200
Turn off "DEBUG" in libparted/libparted.c, ...
thus making it so that ped_malloc no longer initializes all
just-allocated memory to all '1' bits. Given the two bugs
I've just fixed, this change is long overdue.
diff --git a/libparted/libparted.c b/libparted/libparted.c
index b1a6201..e70f187 100644
--- a/libparted/libparted.c
+++ b/libparted/libparted.c
@@ -52,6 +52,10 @@ typedef struct
size_t size;
} pointer_size_type;
+/* IMHO, none of the DEBUG-related code below is useful, and the
+ ped_malloc memset code is actually quite harmful: it masked at
+ least two nasty bugs that were fixed in June of 2007. */
+#undef DEBUG
#ifdef DEBUG
static pointer_size_type dodgy_malloc_list[] = {
{0, 0},
@@ -340,4 +344,3 @@ ped_free (void* ptr)
free (ptr);
}
-
commit 439d3fecf9b06ae24d7f1575d78ae08b95b1bdc3
Author: Jim Meyering <jim at meyering.net>
Date: Tue Jun 5 16:01:46 2007 +0200
Make "mklabel amiga" work also when DEBUG is not enabled.
I turned off DEBUG and discovered test failures that its
bogus always-initialize-malloc'd-memory policy had been hiding:
(amiga_write): Initialize all of "->disk_specific" buffer.
Avoid buffer overrun when initializing "TABLE".
diff --git a/libparted/labels/rdb.c b/libparted/labels/rdb.c
index 483a292..06d1f8b 100644
--- a/libparted/labels/rdb.c
+++ b/libparted/labels/rdb.c
@@ -27,6 +27,10 @@
#include <parted/debug.h>
#include <parted/endian.h>
+#ifndef MAX
+# define MAX(a,b) ((a) < (b) ? (b) : (a))
+#endif
+
#if ENABLE_NLS
# include <libintl.h>
# define _(String) dgettext (PACKAGE, String)
@@ -656,6 +660,11 @@ amiga_write (const PedDisk* disk)
/* Let's read the rdb */
if ((rdb_num = _amiga_find_rdb (disk->dev, rdb)) == AMIGA_RDB_NOT_FOUND) {
rdb_num = 2;
+ size_t pb_size = sizeof (struct PartitionBlock);
+ /* Initialize only the part that won't be copied over
+ with a partition block in amiga_read. */
+ memset ((char *)(RDSK(disk->disk_specific)) + pb_size,
+ 0, PED_SECTOR_SIZE_DEFAULT - pb_size);
} else {
memcpy (RDSK(disk->disk_specific), rdb, PED_SECTOR_SIZE_DEFAULT);
}
@@ -668,13 +677,19 @@ amiga_write (const PedDisk* disk)
last_hb = (PedSector) PED_BE32_TO_CPU (rdb->rdb_RDBBlocksHi);
last_used_hb = (PedSector) PED_BE32_TO_CPU (rdb->rdb_HighRDSKBlock);
- /* let's allocate a free block table and initialize it */
- if (!(table = ped_malloc ((last_hb - first_hb + 1) * sizeof(uint32_t))))
+ /* Allocate a free block table and initialize it.
+ There must be room for at least RDB_NUM + 2 entries, since
+ the first RDB_NUM+1 entries get IDNAME_RIGIDDISK, and the
+ following one must have LINK_END to serve as sentinel. */
+ size_t tab_size = 2 + MAX (last_hb - first_hb, rdb_num);
+ if (!(table = ped_malloc (tab_size * sizeof *table)))
return 0;
- memset(table, 0xff, (last_hb - first_hb + 1) * sizeof(uint32_t));
- for (i = 0; i<=rdb_num; i++) table[i] = IDNAME_RIGIDDISK;
-
+ for (i = 0; i <= rdb_num; i++)
+ table[i] = IDNAME_RIGIDDISK;
+ for ( ; i < tab_size; i++)
+ table[i] = LINK_END;
+
/* Let's allocate a partition block */
if (!(block = ped_malloc (PED_SECTOR_SIZE_DEFAULT))) {
ped_free (table);
commit 3193dca89d9782c85055e3328e82363c671bdd94
Author: Jim Meyering <jim at meyering.net>
Date: Tue Jun 5 12:23:00 2007 +0200
"mklabel bsd": don't read/write initialized memory, with DEBUG turned off
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)
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);
More information about the Parted-commits
mailing list