Re: _primary_constraint(): start_geom is uninitialized if min_geom==0 && cylinder_size < dev->length. Debian bug #602568

[Jim Meyering]

Jean-Christian de Rivaz wrote:
> Jean-Christian de Rivaz a écrit :
>> The first 5MB device have, acoording to parted-2.3, a cylinder size
>> (16065) not smaller than the device length (10240), causing the
>> PedGeometry start_geom of the function _primary_constraint() (file
>> libparted/labels/dos.c) to stay uninitialized. I suspect that the
>> condition "cylinder_size < dev->length" have bee introduced for the
>> memory-mapped test as explained into the comment above the
>> condition. Unfortunately, it seem that the true real world contain
>> at least a type of USB key sold with a storage device that go wrong
>> with this condition.
>
> It seem that the only maintained source archive for GNU parted is
> git://git.debian.org/git/parted/parted.git . Right ?

That is the so-called upstream repository.

> I found the this bug was introduced by this commit:

Thank you for the bug report and for tracking down the
offending commit.

That change ensures that this unusual condition is
caught early enough.  Before that change, passing a negative
length to ped_geometry_init would have triggered a failed
assertion or something similarly unpleasant.
Thus, simply reverting it just reintroduces the original problem.

> commit c79d91ec71882a1673daae0482aa90c514c63cc1
> Author: Jim Meyering <address@hidden>
> Date:   Tue Mar 30 16:50:59 2010 +0200
>
>     dos: accommodate very small devices (useful for testing)
>
>     * libparted/labels/dos.c (_primary_constraint): Don't pass a negative
>     "device_length" to ped_geometry_init when the underlying device
>     has fewer sectors than a "cylinder".

Here's a patch to solve the problem.
It relaxes the constraint to use a starting sector of "1",
if necessary.

Please let me know if it works for you:

>From 9f5b0608611eed40ef33be2096f5d482710602e5 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Tue, 9 Nov 2010 10:25:36 +0100
Subject: [PATCH] dos: fix a bug affecting very small devices (smaller than 1 
cylinder)

This bug was introduced in commit c79d91ec, "dos: accommodate very
small devices (useful for testing)".
* libparted/labels/dos.c (_primary_constraint): The bug was to
skip setting start_geom for small devices.  That led to a used-
uninitialized bug in the subsequent ped_constraint_new call.
The fix is to relax the constraint to use a starting sector of "1",
if necessary.  Report and diagnosis by Jean-Christian de Rivaz in
http://thread.gmane.org/gmane.comp.gnu.parted.bugs/10178
* NEWS (Bug fixes): Mention it.
---
 NEWS                   |    4 ++++
 libparted/labels/dos.c |   14 +++++++-------
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/NEWS b/NEWS
index 5dd7dca..4979b90 100644
--- a/NEWS
+++ b/NEWS
@@ -14,6 +14,10 @@ GNU parted NEWS                                    -*- 
outline -*-
   libparted now recognizes scsi disks with a high major (128-135) as scsi
   disks

+  an msdos partition table on a very small device (smaller than one cylinder)
+  is now recognized.  [bug introduced in parted-2.2]
+
+
 * Noteworthy changes in release 2.3 (2010-05-28) [stable]

 ** New features
diff --git a/libparted/labels/dos.c b/libparted/labels/dos.c
index d9e7d4a..578180b 100644
--- a/libparted/labels/dos.c
+++ b/libparted/labels/dos.c
@@ -1713,13 +1713,13 @@ _primary_constraint (const PedDisk* disk, const 
PedCHSGeometry* bios_geom,
                                        dev->length - min_geom->end))
                        return NULL;
        } else {
-               /* Do not assume that length is larger than 1 cylinder's
-                  worth of sectors.  This is useful when testing with
-                  a memory-mapped "disk" (a la scsi_debug) that is say,
-                  2048 sectors long.  */
-               if (cylinder_size < dev->length
-                   && !ped_geometry_init (&start_geom, dev, cylinder_size,
-                                          dev->length - cylinder_size))
+               /* Use cylinder_size as the starting sector number
+                  when the device is large enough to accommodate that.
+                  Otherwise, use sector 1.  */
+               PedSector start = (cylinder_size < dev->length
+                                  ? cylinder_size : 1);
+               if (!ped_geometry_init (&start_geom, dev, start,
+                                       dev->length - start))
                        return NULL;
                if (!ped_geometry_init (&end_geom, dev, 0, dev->length))
                        return NULL;
--
1.7.3.2.2.gaf77a