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

[Jim Meyering]

Hans de Goede wrote:
> * libparted/arch/linux.c (_dm_add_partition): Fix use of dm_task
> information after freeing it.
> ---
>  libparted/arch/linux.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/libparted/arch/linux.c b/libparted/arch/linux.c
> index 9d15bf2..36a698d 100644
> --- a/libparted/arch/linux.c
> +++ b/libparted/arch/linux.c
> @@ -2447,12 +2447,14 @@ _dm_add_partition (PedDisk* disk, PedPartition* part)
>                  goto err;
>
>          dev_name = dm_task_get_name (task);
> -        dm_task_destroy (task);
> -        task = NULL;
>
>          if (asprintf (&vol_name, "%sp%d", dev_name, part->num) == -1)
>                  goto err;
>
> +        dm_task_destroy(task);
> +        dev_name = NULL;
> +        task = NULL;
> +

Hi Hans,
Thanks for that patch!

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.

>From 71fa59e78799589bed5b85baa7ba2c9efabc661e Mon Sep 17 00:00:00 2001
From: Hans de Goede <address@hidden>
Date: Fri, 18 Dec 2009 10:33:18 +0100
Subject: [PATCH] linux: fix use-after-free in devicemapper code

* libparted/arch/linux.c (_dm_add_partition): Fix use of dm_task
information after freeing it.
---
 libparted/arch/linux.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/libparted/arch/linux.c b/libparted/arch/linux.c
index 9d15bf2..d996f32 100644
--- a/libparted/arch/linux.c
+++ b/libparted/arch/linux.c
@@ -2447,12 +2447,13 @@ _dm_add_partition (PedDisk* disk, PedPartition* part)
                 goto err;

         dev_name = dm_task_get_name (task);
-        dm_task_destroy (task);
-        task = NULL;

         if (asprintf (&vol_name, "%sp%d", dev_name, part->num) == -1)
                 goto err;

+        /* Caution: dm_task_destroy frees dev_name.  */
+        dm_task_destroy (task);
+
         if (asprintf (&params, "%d:%d %lld", arch_specific->major,
                       arch_specific->minor, part->geom.start) == -1)
                 goto err;
--
1.6.6.rc3.271.g3d40f