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

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; }