[parted-devel] Patch: Fix gpt_read to read all of the partition entries correctly

[Joel Granados]

On Sun, Dec 21, 2008 at 09:17:30AM +0100, Jim Meyering wrote:
> "Matthew S. Harris" <mharris312 at gmail.com> wrote:
> > Here's my one remaining fix.  Yes, I've been sitting on this one since
> > April 2007 too.  Hopefully it's clear why this one is important.
> 
> Thanks!
> I don't want to block on test cases,
> but it'd sure make it easier to take a patch that
> comes with a small test case to exercise/demonstrate the fix.

I think that this is not "easily" testable with the tools we have at our
disposal.  We would have to create a gpt table that has partition
entries that exceed the "default" size.  I can think of two ways of
doing this, neither of which is pretty, and I would like to avoid them:
1. To hack parted to create a gpt partition that actually has a reserved
section in the entry array, create an image and put that image in the
src (yuck!!!)
2. To use another tool to create the image and put the image in the src
(yuck!!!)

In any case we would have to put an image in the src, which I consider
less then ideal.

I vote for patching parted with Jim's comments.

> 
> For example, if you can construct a tiny image,
> describe how to perturb it, and give a parted
> command that misbehaves as a result, yet that works
> with your patch, that'd be great.
> 
> > From 282be216499436242cdb3083c0e5d9b672db8682 Mon Sep 17 00:00:00 2001
> > From: Matthew S. Harris <mharris312 at gmail.com>
> > Date: Tue, 2 Dec 2008 19:53:08 -0800
> > Subject: [PATCH] Fix gpt_read to read all of the partition entries correctly.
> >
> > * libparted/labels/gpt.c (gpt_read): Use the SizeOfPartitionEntry
> > field value when reading the partition entries rather than assuming
> > that the entries are the same size as our struct.
> >
> > * libparted/labels/gpt.c (gpt_read): When reading the partition
> > entries, round up, not down, the number of sectors to read.
> > ---
> >  libparted/labels/gpt.c |   17 ++++++++++-------
> >  1 files changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/libparted/labels/gpt.c b/libparted/labels/gpt.c
> > index 13d2e88..892e921 100644
> > --- a/libparted/labels/gpt.c
> > +++ b/libparted/labels/gpt.c
> > @@ -807,8 +807,8 @@ gpt_read (PedDisk * disk)
> >  {
> >  	GPTDiskData *gpt_disk_data = disk->disk_specific;
> >  	GuidPartitionTableHeader_t* gpt;
> > -	GuidPartitionEntry_t* ptes;
> > -	int ptes_size;
> > +	void* ptes;
> > +	int ptes_sectors;
> >  	int i;
> >  #ifndef DISCOVER_ONLY
> >  	int write_back = 0;
> > @@ -889,21 +889,24 @@ gpt_read (PedDisk * disk)
> >  		goto error_free_gpt;
> >
> >
> > -	ptes_size = sizeof (GuidPartitionEntry_t) * gpt_disk_data->entry_count;
> > -	ptes = (GuidPartitionEntry_t*) ped_malloc (ptes_size);
> > +	ptes_sectors = ped_div_round_up(gpt->SizeOfPartitionEntry *
> > +		gpt_disk_data->entry_count, disk->dev->sector_size);
> 
> Does something already ensure that SizeOfPartitionEntry is sane?
> i.e., what if the on-disk value is invalid?
> Maybe add a sanity check, if there isn't one already.
> 
> > +	ptes = ped_malloc (ptes_sectors * disk->dev->sector_size);
> 
> Before doing the malloc, ensure that the product does not overflow,
> e.g.,
> 
>   if (xalloc_oversized (ptes_sectors, disk->dev->sector_size)
>       goto ...;
> 
> >  	if (!ped_device_read (disk->dev, ptes,
> >  			      PED_LE64_TO_CPU(gpt->PartitionEntryLBA),
> > -			      ptes_size / disk->dev->sector_size))
> > +			      ptes_sectors))
> >  		goto error_free_ptes;
> >
> >  	for (i = 0; i < gpt_disk_data->entry_count; i++) {
> > +		GuidPartitionEntry_t* pte = (GuidPartitionEntry_t*) (ptes +
> > +			i * gpt->SizeOfPartitionEntry);
> >  		PedPartition* part;
> >  		PedConstraint* constraint_exact;
> >
> > -		if (!guid_cmp (ptes[i].PartitionTypeGuid, UNUSED_ENTRY_GUID))
> > +		if (!guid_cmp (pte->PartitionTypeGuid, UNUSED_ENTRY_GUID))
> >  			continue;
> >
> > -		part = _parse_part_entry (disk, &ptes[i]);
> > +		part = _parse_part_entry (disk, pte);
> >  		if (!part)
> >  			goto error_delete_all;
> 
> _______________________________________________
> parted-devel mailing list
> parted-devel at lists.alioth.debian.org
> http://lists.alioth.debian.org/mailman/listinfo/parted-devel

-- 
Joel Andres Granados
Brno, Czech Republic, Red Hat.