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

[Jim Meyering]

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/

> +	 */
> +	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.

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

> +	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".

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