Re: [PATCH i-g-t 3/7] lib: Stop igt_get_all_cairo_formats memory leak

[Gabriel Krisman Bertazi <krisman@collabora.co.uk>]

Brian Starkey <brian.starkey@arm.com> writes:

> igt_get_all_cairo_formats() allocates the format list on the heap, but
> returns it in a const pointer. Change this so that callers can free the
> array without a warning, and free the array in all callers.
>
> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> ---
>  lib/igt_fb.c       |    4 +++-
>  lib/igt_fb.h       |    2 +-
>  tests/kms_atomic.c |    3 ++-
>  tests/kms_render.c |    4 +++-
>  4 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index d2b7e9e36606..b958c970973b 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -1277,8 +1277,10 @@ const char *igt_format_str(uint32_t drm_format)
>   *
>   * This functions returns an array of all the drm fourcc codes supported by
>   * cairo and this library.
> + *
> + * The array should be freed by the caller.
>   */
> -void igt_get_all_cairo_formats(const uint32_t **formats, int *format_count)
> +void igt_get_all_cairo_formats(uint32_t **formats, int *format_count)
>  {
>  	static uint32_t *drm_formats;
>  	static int n_formats;
> diff --git a/lib/igt_fb.h b/lib/igt_fb.h
> index 4a680cefb16d..e124910367a3 100644
> --- a/lib/igt_fb.h
> +++ b/lib/igt_fb.h
> @@ -154,7 +154,7 @@ int igt_cairo_printf_line(cairo_t *cr, enum igt_text_align align,
>  uint32_t igt_bpp_depth_to_drm_format(int bpp, int depth);
>  uint32_t igt_drm_format_to_bpp(uint32_t drm_format);
>  const char *igt_format_str(uint32_t drm_format);
> -void igt_get_all_cairo_formats(const uint32_t **formats, int *format_count);
> +void igt_get_all_cairo_formats(uint32_t **formats, int *format_count);
>  
>  #endif /* __IGT_FB_H__ */
>  
> diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c
> index 6375fede7179..9c03f6e21ebb 100644
> --- a/tests/kms_atomic.c
> +++ b/tests/kms_atomic.c
> @@ -807,7 +807,7 @@ static void atomic_state_free(struct kms_atomic_state *state)
>  static uint32_t plane_get_igt_format(struct kms_atomic_plane_state *plane)
>  {
>  	drmModePlanePtr plane_kms;
> -	const uint32_t *igt_formats;
> +	uint32_t *igt_formats;
>  	uint32_t ret = 0;
>  	int num_igt_formats;
>  	int i;
> @@ -827,6 +827,7 @@ static uint32_t plane_get_igt_format(struct kms_atomic_plane_state *plane)
>  		}
>  	}
>  
> +	free(igt_formats);
>  	drmModeFreePlane(plane_kms);
>  	return ret;
>  }
> diff --git a/tests/kms_render.c b/tests/kms_render.c
> index fd33dfb7cafe..bc2ffc750c67 100644
> --- a/tests/kms_render.c
> +++ b/tests/kms_render.c
> @@ -176,7 +176,7 @@ static void test_connector(const char *test_name,
>  			   struct kmstest_connector_config *cconf,
>  			   enum test_flags flags)
>  {
> -	const uint32_t *formats;
> +	uint32_t *formats;
>  	int format_count;
>  	int i;
>  
> @@ -193,6 +193,8 @@ static void test_connector(const char *test_name,
>  			    cconf, &cconf->connector->modes[0],
>  			    formats[i], flags);
>  	}
> +
> +	free(formats);

Hi,

Assuming I'm not missing anything, I think if you free formats here, the
static variable in igt_get_all_cairo_formats() will point to garbage and
further calls to that function will return uninitialized memory.



-- 
Gabriel Krisman Bertazi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx