[parted-devel] bug: parted gets partitions wrong

[Jim Meyering]

Colin Watson <cjwatson at ubuntu.com> wrote:
...
> 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++) {

Thanks for all the analysis and the bug fix.

I confirmed that following code in msdos.c was improperly rejecting
the partition table due to the presence of the string "FAT" at a
"well-known" offset:

-	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;

[checking the MBR at offset 0x52, you do see "FAT":

  $ od -taz -An --skip=82 --read=5 /tmp/sdb-2KB
     F   A   T   3   2                                              >FAT32<

and that your patch works.

I agree on the copyright front.
I'm debating how to test for this.
I'll probably push your patch tomorrow.