[parted-devel] Bug: Removal of BLKPG causes regression of ability to manipulate disks with other partitions in use

[Jim Meyering]

Petr Uzel wrote:
> On Tue, Mar 30, 2010 at 03:37:38PM -0400, Phillip Susi wrote:
>
> [...]
>> +                        if (!rets[i - 1] && errnums[i - 1] == EBUSY) {
>> +                                struct hd_geometry geom;
>> +                                int fd;
>> +                                unsigned long long length = 0;
>> +                                /* get start and length of existing partition */
>> +                                char *dev_name = _device_get_part_path (disk->dev, i);
>> +                                if (!dev_name)
>> +                                        goto free_errnums;
>> +                                fd = open (dev_name, O_RDONLY);
>> +                                free (dev_name);
>> +                                if (fd == -1 ||
>> +                                    ioctl (fd, HDIO_GETGEO, &geom) ||
>> +                                    ioctl (fd, BLKGETSIZE64, &length)) {
>> +                                        ped_exception_throw (
>> +                                                             PED_EXCEPTION_BUG,
>> +                                                             PED_EXCEPTION_CANCEL,
>> +                                                             _("Unable to determine the size and length of %s."),
>> +                                                             dev_name);
>
> Here, dev_name is used after it has been freed.                 ^^^^

Good catch.
Thanks Petr.

Phillip, I presume you've rebased, correcting this and the
typo Petr spotted?  If so, please repost.

Sorry about my delay in looking at this.
I've been busy making upstream grep releases, among
other things ;-)

You'll notice how quickly Hans De Goede's fix went in,
even though it was posted long after yours.  That's because
his patch came with a test case.  True, it's extra work (sometimes a
lot of it), but this is the only sure way to prevent future regressions.

>> How do these look?  By the way, why are we using ped_malloc() and not
>> ped_free()?

ped_free (now gone) used to be a very thin wrapper around free.
I removed it a couple years ago.

>> Passing a pointer to free() that did not come from malloc()
>> is an error, so only works as long as ped_malloc() is just a wrapper for
>> malloc(), and if that has to be the case, what is the point of ped_malloc()?

Upon failed malloc, ped_malloc throws one of parted's kludgey exceptions.
If the ped_malloc-calling code already handles a NULL-return,
then it'd be better just to use malloc.