Re: [PATCH parted 2/2] linux: Fix use after free in devicemapper code

[Hans de Goede]

Hi,

On 12/18/2009 01:59 PM, Jim Meyering wrote:
> Hans de Goede wrote:
> > > Is this adjustment ok with you, since I'm leaving your name on it?
> > > I don't want to leave the assignments to NULL, since they're
> > > effectively dead code and I don't want to provoke warnings
> > > from the likes of clang and coverity.
> > >
> > > I understand the motivation in setting them to NULL,
> > > but since there is obviously no further use of dev_name,
> > > and the next use of "task" is an assignment, it seems safe.
> >
> > You cannot remove the "task = NULL" statement, otherwise the task will
> > get destroyed a second time in the error path of the asprintf directly
> > below the moved "dm_task_destroy (task)"
>
> Oh.  Duh ;-)
>
> I'll fold this into it, also removing a useless initialization.

Ack.

Regards,

Hans


>  From d1f4354cc2bd3ffae2afbb510bafd43f2b6857ba Mon Sep 17 00:00:00 2001
> From: Jim Meyering<address@hidden>
> Date: Fri, 18 Dec 2009 13:51:41 +0100
> Subject: [PATCH] .
>
> ---
>   libparted/arch/linux.c |    4 ++--
>   1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libparted/arch/linux.c b/libparted/arch/linux.c
> index d996f32..aefe788 100644
> --- a/libparted/arch/linux.c
> +++ b/libparted/arch/linux.c
> @@ -2423,7 +2423,6 @@ err:
>   static int
>   _dm_add_partition (PedDisk* disk, PedPartition* part)
>   {
> -        struct dm_task* task = NULL;
>           int             rc;
>           char*           vol_name = NULL;
>           const char*     dev_name = NULL;
> @@ -2434,7 +2433,7 @@ _dm_add_partition (PedDisk* disk, PedPartition* part)
>                   return 0;
>
>           /* Get map name from devicemapper */
> -        task = dm_task_create (DM_DEVICE_INFO);
> +        struct dm_task *task = dm_task_create (DM_DEVICE_INFO);
>           if (!task)
>                   goto err;
>
> @@ -2453,6 +2452,7 @@ _dm_add_partition (PedDisk* disk, PedPartition* part)
>
>           /* Caution: dm_task_destroy frees dev_name.  */
>           dm_task_destroy (task);
> +        task = NULL;
>
>           if (asprintf (&params, "%d:%d %lld", arch_specific->major,
>                         arch_specific->minor, part->geom.start) == -1)
> --
> 1.6.6.rc3.271.g3d40f