[Mesa-dev] [PATCH v4 1/2] gallium/winsys/kms: Fix possible leak in map/unmap.

[Tomasz Figa]

On Thu, Mar 8, 2018 at 7:39 AM, Lepton Wu <lepton at chromium.org> wrote:
> If user calls map twice for kms_sw_displaytarget, the first mapped
> buffer could get leaked. Instead of calling mmap every time, just
> reuse previous mapping. Since user could map same displaytarget with
> different flags, we have to keep two different pointers, one for rw
> mapping and one for ro mapping.
>
> Change-Id: I65308f0ff2640bd57b2577c6a3469540c9722859
> Signed-off-by: Lepton Wu <lepton at chromium.org>
> ---
>  .../winsys/sw/kms-dri/kms_dri_sw_winsys.c     | 21 ++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
> index 22e1c936ac5..7fc40488c2e 100644
> --- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
> +++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
> @@ -70,6 +70,7 @@ struct kms_sw_displaytarget
>
>     uint32_t handle;
>     void *mapped;
> +   void *ro_mapped;
>
>     int ref_count;
>     struct list_head link;
> @@ -198,16 +199,19 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
>        return NULL;
>
>     prot = (flags == PIPE_TRANSFER_READ) ? PROT_READ : (PROT_READ | PROT_WRITE);
> -   kms_sw_dt->mapped = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
> -                            kms_sw->fd, map_req.offset);
> -
> -   if (kms_sw_dt->mapped == MAP_FAILED)
> -      return NULL;
> +   void **ptr = (flags == PIPE_TRANSFER_READ) ? &kms_sw_dt->ro_mapped : &kms_sw_dt->mapped;
> +   if (!*ptr) {
> +      void *tmp = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
> +                       kms_sw->fd, map_req.offset);
> +      if (tmp == MAP_FAILED)
> +         return NULL;
> +      *ptr = tmp;
> +   }
>
>     DEBUG_PRINT("KMS-DEBUG: mapped buffer %u (size %u) at %p\n",
> -         kms_sw_dt->handle, kms_sw_dt->size, kms_sw_dt->mapped);
> +         kms_sw_dt->handle, kms_sw_dt->size, *ptr);
>
> -   return kms_sw_dt->mapped;
> +   return *ptr;
>  }
>
>  static struct kms_sw_displaytarget *
> @@ -278,9 +282,12 @@ kms_sw_displaytarget_unmap(struct sw_winsys *ws,
>     struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
>
>     DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->mapped);
> +   DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->ro_mapped);
>
>     munmap(kms_sw_dt->mapped, kms_sw_dt->size);
>     kms_sw_dt->mapped = NULL;
> +   munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
> +   kms_sw_dt->ro_mapped = NULL;
>  }

If user calls map twice, wouldn't they also call unmap twice?
Moreover, wouldn't the pointer be expected to be still valid between
first and second unmap?

The answer obviously depends on how the API is designed, but i feels
really weird being asymmetrical like that. Typically the mapping would
be refcounted and maps would have to be balanced with unmaps to free
the mapping.

Best regards,
Tomasz