[parted-devel] [PATCH 3/9] Fix gpt_read to read all of the partition entries correctly.

[Joel Granados]

On Fri, Jun 05, 2009 at 02:43:21PM +0200, Jim Meyering wrote:
> Joel Granados Moreno wrote:
> 
> > From: Matthew S. Harris <mharris312 at gmail.com>
> >
> > * 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 (_header_is_valid): Check that the
> >   SizeOfPartitionEntry is sane.
> > ---
> >  libparted/labels/gpt.c |   29 ++++++++++++++++++++++-------
> >  1 files changed, 22 insertions(+), 7 deletions(-)
> >
> > diff --git a/libparted/labels/gpt.c b/libparted/labels/gpt.c
> > index 89e46fc..4597346 100644
> > --- a/libparted/labels/gpt.c
> > +++ b/libparted/labels/gpt.c
> > @@ -38,6 +38,7 @@
> >  #include <unistd.h>
> >  #include <uuid/uuid.h>
> >  #include <stdbool.h>
> > +#include "xalloc.h"
> >
> >  #if ENABLE_NLS
> >  #  include <libintl.h>
> > @@ -603,6 +604,14 @@ _header_is_valid (const PedDevice* dev, GuidPartitionTableHeader_t* gpt)
> >  	    || PED_LE32_TO_CPU (gpt->HeaderSize) > dev->sector_size)
> >  		return 0;
> >
> > +	/*
> > +	 * the SizeOfPartitionEntry must be a multiple of 8 and
> > +	 * greater than the size of the PartitionEntry structure.
> 
> off-by-one comment:
> s/greater/no smaller than/

Yep,

> 
> > +	 */
> > +	uint32_t sope = gpt->SizeOfPartitionEntry;
> > +	if (sope % 8 != 0 || sope < sizeof(GuidPartitionEntry_t) )
> > +		return 0;
> > +
> >  	origcrc = gpt->HeaderCRC32;
> >  	gpt->HeaderCRC32 = 0;
> >  	crc = pth_crc32 (dev, gpt);
> > @@ -807,8 +816,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;
> 
> Sizes and counts must be unsigned, whenever possible.
> Please use size_t.
> 

ok.

> >  	int i;
> >  #ifndef DISCOVER_ONLY
> >  	int write_back = 0;
> > @@ -902,22 +911,28 @@ gpt_read (PedDisk * disk)
> >  	if (!_parse_header (disk, gpt, &write_back))
> >  		goto error_free_gpt;
> >
> > +	ptes_sectors = ped_div_round_up(gpt->SizeOfPartitionEntry *
> > +		gpt_disk_data->entry_count, disk->dev->sector_size);
> 
> Please format continued expressions per GNU coding guidelines, e.g.,
> (i.e., aligned with relevant paren, operator at beginning of
> 2nd and subsequent continued line, not at end of preceding):
> 
>     	ptes_sectors = ped_div_round_up(gpt->SizeOfPartitionEntry
>                                         * gpt_disk_data->entry_count,
>                                         disk->dev->sector_size));
> 

sure

> > +	if (xalloc_oversized (ptes_sectors, disk->dev->sector_size))
> > +		goto error_free_gpt;
> > +	ptes = ped_malloc (ptes_sectors * disk->dev->sector_size);
> >
> > -	ptes_size = sizeof (GuidPartitionEntry_t) * gpt_disk_data->entry_count;
> > -	ptes = (GuidPartitionEntry_t*) ped_malloc (ptes_size);
> >  	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);
> 
> Same here:
> 
>     		GuidPartitionEntry_t* pte
>                   = (GuidPartitionEntry_t*) (ptes + i
>                                              * gpt->SizeOfPartitionEntry);
> 
> Hmm... since ptes is a void* pointer,
> doesn't the above elicit a warning from gcc?
> void pointer arithmetic is not portable.
> You'll want to cast it to "(char *)" first.
> 
> Please ensure that your changes do not introduce new warnings,
> and that the result passes "make distcheck".

Very helpfull ^^^^^

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

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