[parted-devel] bug: parted gets partitions wrong

[Colin Watson]

On Fri, May 23, 2008 at 10:35:36PM +0200, Jim Meyering wrote:
> Jim Meyering <jim at meyering.net> wrote:
> > "xerces8" <xerces8 at butn.net> wrote:
> >>> The first 4 sectors are attached.
> >
> > Thanks!
> > I'll take a look by Friday.
> 
> Thanks.
> I did reproduce the failure (fdisk sees 2 partitions, parted sees just
> one), using your MBR on a 0-extended loopback device.
> However, that's not fair, since loopback doesn't work with partitions.
> Then I put the boot sector on a USB key.  fdisk still sees two:
> 
>     $ fdisk -l /dev/sdc
> 
>     Disk /dev/sdc: 65 MB, 65986560 bytes
>     64 heads, 32 sectors/track, 62 cylinders
>     Units = cylinders of 2048 * 512 = 1048576 bytes
>     Disk identifier: 0x00000000
> 
>        Device Boot      Start         End      Blocks   Id  System
>     /dev/sdc1   *           1         716      733168    6  FAT16
>     /dev/sdc2             717         979      269312    6  FAT16
> 
> but parted fails:
> 
>     $ ./parted -m -s /dev/sdc p
>     Error: /dev/sdc: unrecognised disk label
>     [Exit 1]
> 
> I suspect that the above merely shows that parted
> looks farther than the partition table, and attempts
> to do some minimal verification.  When (at least in my case)
> it doesn't find anything sane beyond the partition table, it fails.
> I'll investigate further next week.

I investigated this independently today; the reporter didn't say that
he'd taken it here, so I didn't know about this thread until I went and
looked.

The problem appears to be (and this is conjecture) that the disk
originally simply had a FAT file system on it, with no partition table.
When it was given a partition table, its boot sector wasn't replaced,
and so it still has the FAT signature.

It would be more reliable to do what the Linux kernel does: namely,
check whether the boot indicator on each partition is 0 or 0x80, and
otherwise assume the disk is a FAT file system. Here's the Linux code
for comparison:

        /*
         * Now that the 55aa signature is present, this is probably
         * either the boot sector of a FAT filesystem or a DOS-type
         * partition table. Reject this in case the boot indicator
         * is not 0 or 0x80.
         */
        p = (struct partition *) (data + 0x1be);
        for (slot = 1; slot <= 4; slot++, p++) {
                if (p->boot_ind != 0 && p->boot_ind != 0x80) {
                        put_dev_sector(sect);
                        return 0;
                }
        }

util-linux's fdisk has a similar check in the is_garbage_table function:

        for (i = 0; i < 4; i++) {
                struct pte *pe = &ptes[i];
                struct partition *p = pe->part_table;

                if (p->boot_ind != 0 && p->boot_ind != 0x80)
                        return 1;
        }
        return 0;

Please consider the following patch, which does basically the same
thing. I think it's sufficiently trivial (and obvious based on other
implementations) to be non-copyrightable, but I can do assignment
paperwork if you feel you need it.

With this patch, the supplied boot sector prints as follows:

  $ LD_LIBRARY_PATH=`pwd`/libparted/.libs parted/.libs/parted ~/parted-test
  WARNING: You are not superuser.  Watch out for permissions.
  GNU Parted 1.8.8.1.30-c31f
  Using /home/cjwatson/parted-test
  Welcome to GNU Parted! Type 'help' to view a list of commands.
  (parted) print
  Model:  (file)
  Disk /home/cjwatson/parted-test: 1573MB
  Sector size (logical/physical): 512B/512B
  Partition Table: msdos
  
  Number  Start   End     Size   Type     File system  Flags
   1      16.4kB  751MB   751MB  primary               boot
   2      751MB   1027MB  276MB  primary

Signed-off-by: Colin Watson <cjwatson at canonical.com>

---
 dos.c |   16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/libparted/labels/dos.c b/libparted/labels/dos.c
index e513a05..81d8600 100644
--- a/libparted/labels/dos.c
+++ b/libparted/labels/dos.c
@@ -192,14 +192,16 @@ msdos_probe (const PedDevice *dev)
 	if (PED_LE16_TO_CPU (part_table->magic) != MSDOS_MAGIC)
 		goto probe_fail;
 
-	/* if this is a FAT fs, fail here.  Note that the Smart Boot Manager
-	 * Loader (SBML) signature indicates a partition table, not a file
-	 * system.
+	/* If this is a FAT fs, fail here.  Checking for the FAT signature
+	 * has some false positives; instead, do what the Linux kernel does
+	 * and ensure that each partition has a boot indicator that is
+	 * either 0 or 0x80.
 	 */
-	if ((!strncmp (part_table->boot_code + 0x36, "FAT", 3)
-	    && strncmp (part_table->boot_code + 0x40, "SBML", 4) != 0)
-	    || !strncmp (part_table->boot_code + 0x52, "FAT", 3))
-		goto probe_fail;
+	for (i = 0; i < 4; i++) {
+		if (part_table->partitions[i].boot_ind != 0
+		    && part_table->partitions[i].boot_ind != 0x80)
+			goto probe_fail;
+	}
 
 	/* If this is a GPT disk, fail here */
 	for (i = 0; i < 4; i++) {

-- 
Colin Watson                                       [cjwatson at ubuntu.com]