On Thu, 2007-03-08 at 23:41 +0100, Jim Meyering wrote: > David Cantrell <[EMAIL PROTECTED]> wrote: > > > On Thu, 2007-03-08 at 22:26 +0100, Jim Meyering wrote: > >> David Cantrell <[EMAIL PROTECTED]> wrote: > >> > On Thu, 2007-03-08 at 15:43 +0100, Jim Meyering wrote: > >> >> Here's a fix for the first memory overrun bug I found: > >> >> > >> >> aix.c: Avoid memory overrun. Don't assume logical sector size > >> >> <= 512B > >> >> * libparted/labels/aix.c (aix_probe): Return 0 if the > >> >> sector size is larger than our AixLabel size. > >> >> (aix_clobber): Rather than PED_ASSERT'ing that aix_probe > >> >> returns 1, > >> >> simply return 0 if aix_probe returns fails. > >> ... > >> > I like the approach of allocating the required buffer size instead of > >> > using fixed-size buffers. > >> > > >> > Either solution is fine, but I would prefer we go with the ped_malloc() > >> > approach that's used in amiga_probe so we are consistent throughout > >> > libparted. > >> > >> OK. I've reworked it and tested the read-only parts. > ... > > Should we not be using unsigned int where we're using int? > > Yep. Good catch. > Actually, it should probably be uint32_t, > but if "unsigned int" ever ends up with size different > from 32 bits, this will be the least of our problems.
Very true. I would like to think that we're safe with unsigned int and
32-bits for its storage on the platforms we support, but there's always
the corner case.
> Here's an updated patch:
>
> --------------------
> aix.c: Avoid memory overrun. Don't assume logical sector size <= 512B
>
> From: Jim Meyering <[EMAIL PROTECTED]>
>
> * libparted/labels/aix.c (struct AixLabel): Remove definition.
> (aix_label_magic_get, aix_label_magic_set): New functions.
> (read_sector): New function.
> (aix_probe): Rewrite not to use the above, and not a static buffer.
> (aix_clobber): Likewise.
> Also, rather than PED_ASSERT'ing that aix_probe returns 1,
> simply return 0 if aix_probe returns fails.
> (ped_disk_aix_init): Remove assertion, now that AixLabel is gone.
> ---
>
> libparted/labels/aix.c | 64
> ++++++++++++++++++++++++++++++++----------------
> 1 files changed, 43 insertions(+), 21 deletions(-)
>
> diff --git a/libparted/labels/aix.c b/libparted/labels/aix.c
> index a16ead4..9e7a3e2 100644
> --- a/libparted/labels/aix.c
> +++ b/libparted/labels/aix.c
> @@ -33,45 +33,68 @@
> # define _(String) (String)
> #endif /* ENABLE_NLS */
>
> -typedef struct {
> - unsigned int magic; /* expect AIX_LABEL_MAGIC */
> - unsigned int fillbytes[127];
> -} AixLabel;
> -
> #define AIX_LABEL_MAGIC 0xc9c2d4c1
>
> static PedDiskType aix_disk_type;
>
> -static int
> -aix_probe (const PedDevice *dev)
> +static inline int
> +aix_label_magic_get (const char *label)
> {
> - AixLabel label;
> + return *(unsigned int *)label;
> +}
>
> - PED_ASSERT (dev != NULL, return 0);
> +static inline void
> +aix_label_magic_set (char *label, int magic_val)
> +{
> + *(unsigned int *)label = magic_val;
> +}
>
> - if (!ped_device_read (dev, &label, 0, 1))
> +/* Read a single sector, of length DEV->sector_size, into malloc'd storage.
> + If the read fails, free the memory and return zero without modifying *BUF.
> + Otherwise, set *BUF to the new buffer and return 1. */
> +static int
> +read_sector (const PedDevice *dev, char **buf)
> +{
> + char *b = ped_malloc (dev->sector_size);
> + PED_ASSERT (b != NULL, return 0);
> + if (!ped_device_read (dev, b, 0, 1)) {
> + ped_free (b);
> return 0;
> + }
> + *buf = b;
> + return 1;
> +}
>
> - if (PED_BE32_TO_CPU (label.magic) != AIX_LABEL_MAGIC)
> - return 0;
> +static int
> +aix_probe (const PedDevice *dev)
> +{
> + PED_ASSERT (dev != NULL, return 0);
>
> - return 1;
> + char *label;
> + if (!read_sector (dev, &label))
> + return 0;
> + unsigned int magic = aix_label_magic_get (label);
> + ped_free (label);
> + return magic == AIX_LABEL_MAGIC;
> }
>
> #ifndef DISCOVER_ONLY
> static int
> aix_clobber (PedDevice* dev)
> {
> - AixLabel label;
> -
> PED_ASSERT (dev != NULL, return 0);
> - PED_ASSERT (aix_probe (dev), return 0);
>
> - if (!ped_device_read (dev, &label, 0, 1))
> + if (!aix_probe (dev))
> return 0;
> -
> - label.magic = 0;
> - return ped_device_write (dev, &label, 0, 1);
> +
> + char *label;
> + if (!read_sector (dev, &label))
> + return 0;
> +
> + aix_label_magic_set (label, 0);
> + int result = ped_device_write (dev, label, 0, 1);
> + ped_free (label);
> + return result;
> }
> #endif /* !DISCOVER_ONLY */
>
> @@ -263,7 +286,6 @@ static PedDiskType aix_disk_type = {
> void
> ped_disk_aix_init ()
> {
> - PED_ASSERT (sizeof (AixLabel) == 512, return);
> ped_disk_type_register (&aix_disk_type);
> }
ACK.
--
David Cantrell <[EMAIL PROTECTED]>
Red Hat / Westford, MA
signature.asc
Description: This is a digitally signed message part
_______________________________________________ parted-devel mailing list [email protected] http://lists.alioth.debian.org/mailman/listinfo/parted-devel
