[PATCH libdrm] tests/exynos: fix memory issues of error path in g2d test

[PATCH libdrm] tests/exynos: fix memory issues of error path in g2d test

[Seung-Woo Kim]

This patch fixes memory issues including NULL deference and leak
in g2d test in error path.

Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
---
 tests/exynos/exynos_fimg2d_test.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/tests/exynos/exynos_fimg2d_test.c b/tests/exynos/exynos_fimg2d_test.c
index 797fb6e..2177e08 100644
--- a/tests/exynos/exynos_fimg2d_test.c
+++ b/tests/exynos/exynos_fimg2d_test.c
@@ -59,7 +59,6 @@ static void connector_find_mode(int fd, struct connector *c,
 		if (!connector) {
 			fprintf(stderr, "could not get connector %i: %s\n",
 				resources->connectors[i], strerror(errno));
-			drmModeFreeConnector(connector);
 			continue;
 		}
 
@@ -98,7 +97,6 @@ static void connector_find_mode(int fd, struct connector *c,
 		if (!c->encoder) {
 			fprintf(stderr, "could not get encoder %i: %s\n",
 				resources->encoders[i], strerror(errno));
-			drmModeFreeEncoder(c->encoder);
 			continue;
 		}
 
@@ -264,7 +262,8 @@ static int g2d_copy_test(struct exynos_device *dev, struct exynos_bo *src,
 		userptr = (unsigned long)malloc(size);
 		if (!userptr) {
 			fprintf(stderr, "failed to allocate userptr.\n");
-			return -EFAULT;
+			ret = -EFAULT;
+			goto fail;
 		}
 
 		src_img.user_ptr[0].userptr = userptr;
@@ -469,7 +468,8 @@ static int g2d_copy_with_scale_test(struct exynos_device *dev,
 		userptr = (unsigned long)malloc(size);
 		if (!userptr) {
 			fprintf(stderr, "failed to allocate userptr.\n");
-			return -EFAULT;
+			ret = -EFAULT;
+			goto fail;
 		}
 
 		src_img.user_ptr[0].userptr = userptr;
@@ -557,7 +557,8 @@ static int g2d_blend_test(struct exynos_device *dev,
 		userptr = (unsigned long)malloc(size);
 		if (!userptr) {
 			fprintf(stderr, "failed to allocate userptr.\n");
-			return -EFAULT;
+			ret = -EFAULT;
+			goto fail;
 		}
 
 		src_img.user_ptr[0].userptr = userptr;
@@ -755,7 +756,7 @@ int main(int argc, char **argv)
 
 	dev = exynos_device_create(fd);
 	if (!dev) {
-		drmClose(dev->fd);
+		drmClose(fd);
 		return -EFAULT;
 	}
 
-- 
1.7.4.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm] tests/exynos: fix memory issues of error path in g2d test

[Emil Velikov]

Hi Seung-Woo Kim,

On 16 March 2017 at 02:00, Seung-Woo Kim <sw0312.kim@samsung.com> wrote:
> This patch fixes memory issues including NULL deference and leak
> in g2d test in error path.
>
> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
> ---
>  tests/exynos/exynos_fimg2d_test.c |   13 +++++++------
>  1 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/tests/exynos/exynos_fimg2d_test.c b/tests/exynos/exynos_fimg2d_test.c
> index 797fb6e..2177e08 100644
> --- a/tests/exynos/exynos_fimg2d_test.c
> +++ b/tests/exynos/exynos_fimg2d_test.c
> @@ -59,7 +59,6 @@ static void connector_find_mode(int fd, struct connector *c,
>                 if (!connector) {
>                         fprintf(stderr, "could not get connector %i: %s\n",
>                                 resources->connectors[i], strerror(errno));
> -                       drmModeFreeConnector(connector);
>                         continue;
>                 }
>
> @@ -98,7 +97,6 @@ static void connector_find_mode(int fd, struct connector *c,
>                 if (!c->encoder) {
>                         fprintf(stderr, "could not get encoder %i: %s\n",
>                                 resources->encoders[i], strerror(errno));
> -                       drmModeFreeEncoder(c->encoder);
>                         continue;
>                 }
>
> @@ -264,7 +262,8 @@ static int g2d_copy_test(struct exynos_device *dev, struct exynos_bo *src,
>                 userptr = (unsigned long)malloc(size);
>                 if (!userptr) {
>                         fprintf(stderr, "failed to allocate userptr.\n");
> -                       return -EFAULT;
> +                       ret = -EFAULT;
> +                       goto fail;
Even if you have the best of intentions, but there's yet another bug
in the existing code :-\

Namely: we always return 0 even on error - i.e. the "return 0", after
the g2d_fini() must be "return ret"
This applies to all the tests, afaics.


> @@ -755,7 +756,7 @@ int main(int argc, char **argv)
>
>         dev = exynos_device_create(fd);
>         if (!dev) {
> -               drmClose(dev->fd);
> +               drmClose(fd);
Seems correct, but an alternative (better IMHO) solution is to:
 - flip/fix the call drmClose() <> exynos_device_destroy() order in
err_drm_close.
 - use "fd" in exynos_device_destroy's drmClose.
 - add separate label and use it in the above case.

Can you give this a try, please ?

Thanks
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm] tests/exynos: fix memory issues of error path in g2d test

[Seung-Woo Kim]

Hello Emil,

Thanks for comment.

On 2017년 03월 20일 09:08, Emil Velikov wrote:
> Hi Seung-Woo Kim,
> 
> On 16 March 2017 at 02:00, Seung-Woo Kim <sw0312.kim@samsung.com> wrote:
>> This patch fixes memory issues including NULL deference and leak
>> in g2d test in error path.
>>
>> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
>> ---
>>  tests/exynos/exynos_fimg2d_test.c |   13 +++++++------
>>  1 files changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/tests/exynos/exynos_fimg2d_test.c b/tests/exynos/exynos_fimg2d_test.c
>> index 797fb6e..2177e08 100644
>> --- a/tests/exynos/exynos_fimg2d_test.c
>> +++ b/tests/exynos/exynos_fimg2d_test.c
>> @@ -59,7 +59,6 @@ static void connector_find_mode(int fd, struct connector *c,
>>                 if (!connector) {
>>                         fprintf(stderr, "could not get connector %i: %s\n",
>>                                 resources->connectors[i], strerror(errno));
>> -                       drmModeFreeConnector(connector);
>>                         continue;
>>                 }
>>
>> @@ -98,7 +97,6 @@ static void connector_find_mode(int fd, struct connector *c,
>>                 if (!c->encoder) {
>>                         fprintf(stderr, "could not get encoder %i: %s\n",
>>                                 resources->encoders[i], strerror(errno));
>> -                       drmModeFreeEncoder(c->encoder);
>>                         continue;
>>                 }
>>
>> @@ -264,7 +262,8 @@ static int g2d_copy_test(struct exynos_device *dev, struct exynos_bo *src,
>>                 userptr = (unsigned long)malloc(size);
>>                 if (!userptr) {
>>                         fprintf(stderr, "failed to allocate userptr.\n");
>> -                       return -EFAULT;
>> +                       ret = -EFAULT;
>> +                       goto fail;
> Even if you have the best of intentions, but there's yet another bug
> in the existing code :-\
> 
> Namely: we always return 0 even on error - i.e. the "return 0", after
> the g2d_fini() must be "return ret"
> This applies to all the tests, afaics.

You are right. I will fix it.

> 
> 
>> @@ -755,7 +756,7 @@ int main(int argc, char **argv)
>>
>>         dev = exynos_device_create(fd);
>>         if (!dev) {
>> -               drmClose(dev->fd);
>> +               drmClose(fd);
> Seems correct, but an alternative (better IMHO) solution is to:
>  - flip/fix the call drmClose() <> exynos_device_destroy() order in
> err_drm_close.
>  - use "fd" in exynos_device_destroy's drmClose.
>  - add separate label and use it in the above case.

Ok, I will add error label.

> 
> Can you give this a try, please ?

After fixing as your comment, I will send v2.

Regards,
- Seung-Woo Kim

> 
> Thanks
> Emil
> 
> 
> 

-- 
Seung-Woo Kim
Samsung Software R&D Center
--

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] tests/exynos: fix invalid code of error path in g2d test

[Seung-Woo Kim]

This patch fixes invalid code of error path including NULL
deference and leak in g2d test.

Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
---
 tests/exynos/exynos_fimg2d_test.c |   39 +++++++++++++++++++-----------------
 1 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/tests/exynos/exynos_fimg2d_test.c b/tests/exynos/exynos_fimg2d_test.c
index 797fb6e..e4e960c 100644
--- a/tests/exynos/exynos_fimg2d_test.c
+++ b/tests/exynos/exynos_fimg2d_test.c
@@ -59,7 +59,6 @@ static void connector_find_mode(int fd, struct connector *c,
 		if (!connector) {
 			fprintf(stderr, "could not get connector %i: %s\n",
 				resources->connectors[i], strerror(errno));
-			drmModeFreeConnector(connector);
 			continue;
 		}
 
@@ -98,7 +97,6 @@ static void connector_find_mode(int fd, struct connector *c,
 		if (!c->encoder) {
 			fprintf(stderr, "could not get encoder %i: %s\n",
 				resources->encoders[i], strerror(errno));
-			drmModeFreeEncoder(c->encoder);
 			continue;
 		}
 
@@ -264,7 +262,8 @@ static int g2d_copy_test(struct exynos_device *dev, struct exynos_bo *src,
 		userptr = (unsigned long)malloc(size);
 		if (!userptr) {
 			fprintf(stderr, "failed to allocate userptr.\n");
-			return -EFAULT;
+			ret = -EFAULT;
+			goto fail;
 		}
 
 		src_img.user_ptr[0].userptr = userptr;
@@ -469,7 +468,8 @@ static int g2d_copy_with_scale_test(struct exynos_device *dev,
 		userptr = (unsigned long)malloc(size);
 		if (!userptr) {
 			fprintf(stderr, "failed to allocate userptr.\n");
-			return -EFAULT;
+			ret = -EFAULT;
+			goto fail;
 		}
 
 		src_img.user_ptr[0].userptr = userptr;
@@ -520,7 +520,7 @@ err_free_userptr:
 fail:
 	g2d_fini(ctx);
 
-	return 0;
+	return ret;;
 }
 
 static int g2d_blend_test(struct exynos_device *dev,
@@ -557,7 +557,8 @@ static int g2d_blend_test(struct exynos_device *dev,
 		userptr = (unsigned long)malloc(size);
 		if (!userptr) {
 			fprintf(stderr, "failed to allocate userptr.\n");
-			return -EFAULT;
+			ret = -EFAULT;
+			goto fail;
 		}
 
 		src_img.user_ptr[0].userptr = userptr;
@@ -619,7 +620,7 @@ err_free_userptr:
 fail:
 	g2d_fini(ctx);
 
-	return 0;
+	return ret;
 }
 
 static int g2d_checkerboard_test(struct exynos_device *dev,
@@ -645,8 +646,8 @@ static int g2d_checkerboard_test(struct exynos_device *dev,
 	dst_y = 0;
 
 	checkerboard = create_checkerboard_pattern(screen_width / 32, screen_height / 32, 32);
-	if (checkerboard == NULL) {
-		ret = -1;
+	if (!checkerboard) {
+		ret = -EFAULT;
 		goto fail;
 	}
 
@@ -755,8 +756,8 @@ int main(int argc, char **argv)
 
 	dev = exynos_device_create(fd);
 	if (!dev) {
-		drmClose(dev->fd);
-		return -EFAULT;
+		ret = -EFAULT;
+		goto err_drm_close;
 	}
 
 	resources = drmModeGetResources(dev->fd);
@@ -764,7 +765,7 @@ int main(int argc, char **argv)
 		fprintf(stderr, "drmModeGetResources failed: %s\n",
 				strerror(errno));
 		ret = -EFAULT;
-		goto err_drm_close;
+		goto err_dev_destory;
 	}
 
 	connector_find_mode(dev->fd, &con, resources);
@@ -773,7 +774,7 @@ int main(int argc, char **argv)
 	if (!con.mode) {
 		fprintf(stderr, "failed to find usable connector\n");
 		ret = -EFAULT;
-		goto err_drm_close;
+		goto err_dev_destory;
 	}
 
 	screen_width = con.mode->hdisplay;
@@ -782,7 +783,7 @@ int main(int argc, char **argv)
 	if (screen_width == 0 || screen_height == 0) {
 		fprintf(stderr, "failed to find sane resolution on connector\n");
 		ret = -EFAULT;
-		goto err_drm_close;
+		goto err_dev_destory;
 	}
 
 	printf("screen width = %d, screen height = %d\n", screen_width,
@@ -791,7 +792,7 @@ int main(int argc, char **argv)
 	bo = exynos_create_buffer(dev, screen_width * screen_height * 4, 0);
 	if (!bo) {
 		ret = -EFAULT;
-		goto err_drm_close;
+		goto err_dev_destory;
 	}
 
 	handles[0] = bo->handle;
@@ -882,9 +883,11 @@ err_rm_fb:
 err_destroy_buffer:
 	exynos_destroy_buffer(bo);
 
-err_drm_close:
-	drmClose(dev->fd);
+err_dev_destory:
 	exynos_device_destroy(dev);
 
-	return 0;
+err_drm_close:
+	drmClose(fd);
+
+	return ret;
 }
-- 
1.7.4.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] tests/exynos: fix invalid code of error path in g2d test

[Emil Velikov]

On 20 March 2017 at 00:52, Seung-Woo Kim <sw0312.kim@samsung.com> wrote:
> This patch fixes invalid code of error path including NULL
> deference and leak in g2d test.
>
Thanks for the update. R-b and pushed to master.

For future do add v2 in the subject prefix (git send-email -v2 ...).
It makes it easier to find the correct version of the patch.

-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel