* [PATCH libdrm] libkms/exynos: fix memory leak in error path @ 2016-11-14 5:31 Seung-Woo Kim 2016-12-14 8:16 ` [RESEND][PATCH] " Seung-Woo Kim 0 siblings, 1 reply; 6+ messages in thread From: Seung-Woo Kim @ 2016-11-14 5:31 UTC (permalink / raw) To: dri-devel, robclark, inki.dae; +Cc: sw0312.kim This patch fixes memory leak in error path of exynos_bo_create(). Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com> --- libkms/exynos.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/libkms/exynos.c b/libkms/exynos.c index 5de2e5a..0e97fb5 100644 --- a/libkms/exynos.c +++ b/libkms/exynos.c @@ -88,7 +88,8 @@ exynos_bo_create(struct kms_driver *kms, pitch = (pitch + 512 - 1) & ~(512 - 1); size = pitch * ((height + 4 - 1) & ~(4 - 1)); } else { - return -EINVAL; + ret = -EINVAL; + goto err_free; } memset(&arg, 0, sizeof(arg)); -- 1.7.4.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [RESEND][PATCH] libkms/exynos: fix memory leak in error path 2016-11-14 5:31 [PATCH libdrm] libkms/exynos: fix memory leak in error path Seung-Woo Kim @ 2016-12-14 8:16 ` Seung-Woo Kim 2016-12-14 12:07 ` Eric Engestrom 0 siblings, 1 reply; 6+ messages in thread From: Seung-Woo Kim @ 2016-12-14 8:16 UTC (permalink / raw) To: dri-devel, robclark, inki.dae; +Cc: sw0312.kim, emil.l.velikov This patch fixes memory leak in error path of exynos_bo_create(). Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com> --- libkms/exynos.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/libkms/exynos.c b/libkms/exynos.c index 5de2e5a..0e97fb5 100644 --- a/libkms/exynos.c +++ b/libkms/exynos.c @@ -88,7 +88,8 @@ exynos_bo_create(struct kms_driver *kms, pitch = (pitch + 512 - 1) & ~(512 - 1); size = pitch * ((height + 4 - 1) & ~(4 - 1)); } else { - return -EINVAL; + ret = -EINVAL; + goto err_free; } memset(&arg, 0, sizeof(arg)); -- 1.7.4.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RESEND][PATCH] libkms/exynos: fix memory leak in error path 2016-12-14 8:16 ` [RESEND][PATCH] " Seung-Woo Kim @ 2016-12-14 12:07 ` Eric Engestrom 2016-12-14 12:21 ` Emil Velikov 0 siblings, 1 reply; 6+ messages in thread From: Eric Engestrom @ 2016-12-14 12:07 UTC (permalink / raw) To: Seung-Woo Kim; +Cc: emil.l.velikov, robclark, dri-devel On Wednesday, 2016-12-14 17:16:30 +0900, Seung-Woo Kim wrote: > This patch fixes memory leak in error path of exynos_bo_create(). Indeed, thanks! Reviewed-by: Eric Engestrom <eric.engestrom@imgtec.com> > > Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com> > --- > libkms/exynos.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/libkms/exynos.c b/libkms/exynos.c > index 5de2e5a..0e97fb5 100644 > --- a/libkms/exynos.c > +++ b/libkms/exynos.c > @@ -88,7 +88,8 @@ exynos_bo_create(struct kms_driver *kms, > pitch = (pitch + 512 - 1) & ~(512 - 1); > size = pitch * ((height + 4 - 1) & ~(4 - 1)); > } else { > - return -EINVAL; > + ret = -EINVAL; > + goto err_free; > } > > memset(&arg, 0, sizeof(arg)); > -- > 1.7.4.1 > However, I feel like a cleaner fix might be to simply move the allocation to where it's used and remove the now-unnecessary error path, ie.: ----8<---- diff --git a/libkms/exynos.c b/libkms/exynos.c index 5de2e5a..e2c1c9f 100644 --- a/libkms/exynos.c +++ b/libkms/exynos.c @@ -76,10 +76,6 @@ exynos_bo_create(struct kms_driver *kms, } } - bo = calloc(1, sizeof(*bo)); - if (!bo) - return -ENOMEM; - if (type == KMS_BO_TYPE_CURSOR_64X64_A8R8G8B8) { pitch = 64 * 4; size = 64 * 64 * 4; @@ -96,7 +92,11 @@ exynos_bo_create(struct kms_driver *kms, ret = drmCommandWriteRead(kms->fd, DRM_EXYNOS_GEM_CREATE, &arg, sizeof(arg)); if (ret) - goto err_free; + return ret; + + bo = calloc(1, sizeof(*bo)); + if (!bo) + return -ENOMEM; bo->base.kms = kms; bo->base.handle = arg.handle; @@ -106,10 +106,6 @@ exynos_bo_create(struct kms_driver *kms, *out = &bo->base; return 0; - -err_free: - free(bo); - return ret; } static int ---->8---- Bigger change, but cleaner code IMHO. What do you think? Cheers, Eric _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RESEND][PATCH] libkms/exynos: fix memory leak in error path 2016-12-14 12:07 ` Eric Engestrom @ 2016-12-14 12:21 ` Emil Velikov 2016-12-14 13:12 ` Eric Engestrom 0 siblings, 1 reply; 6+ messages in thread From: Emil Velikov @ 2016-12-14 12:21 UTC (permalink / raw) To: Eric Engestrom; +Cc: Seung-Woo Kim, Rob Clark, ML dri-devel On 14 December 2016 at 12:07, Eric Engestrom <eric.engestrom@imgtec.com> wrote: > On Wednesday, 2016-12-14 17:16:30 +0900, Seung-Woo Kim wrote: >> This patch fixes memory leak in error path of exynos_bo_create(). > > Indeed, thanks! > Reviewed-by: Eric Engestrom <eric.engestrom@imgtec.com> > >> >> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com> >> --- >> libkms/exynos.c | 3 ++- >> 1 files changed, 2 insertions(+), 1 deletions(-) >> >> diff --git a/libkms/exynos.c b/libkms/exynos.c >> index 5de2e5a..0e97fb5 100644 >> --- a/libkms/exynos.c >> +++ b/libkms/exynos.c >> @@ -88,7 +88,8 @@ exynos_bo_create(struct kms_driver *kms, >> pitch = (pitch + 512 - 1) & ~(512 - 1); >> size = pitch * ((height + 4 - 1) & ~(4 - 1)); >> } else { >> - return -EINVAL; >> + ret = -EINVAL; >> + goto err_free; >> } >> >> memset(&arg, 0, sizeof(arg)); >> -- >> 1.7.4.1 >> > > However, I feel like a cleaner fix might be to simply move the > allocation to where it's used and remove the now-unnecessary > error path, ie.: > > ----8<---- > diff --git a/libkms/exynos.c b/libkms/exynos.c > index 5de2e5a..e2c1c9f 100644 > --- a/libkms/exynos.c > +++ b/libkms/exynos.c > @@ -76,10 +76,6 @@ exynos_bo_create(struct kms_driver *kms, > } > } > > - bo = calloc(1, sizeof(*bo)); > - if (!bo) > - return -ENOMEM; > - > if (type == KMS_BO_TYPE_CURSOR_64X64_A8R8G8B8) { > pitch = 64 * 4; > size = 64 * 64 * 4; > @@ -96,7 +92,11 @@ exynos_bo_create(struct kms_driver *kms, > > ret = drmCommandWriteRead(kms->fd, DRM_EXYNOS_GEM_CREATE, &arg, sizeof(arg)); > if (ret) > - goto err_free; > + return ret; > + > + bo = calloc(1, sizeof(*bo)); > + if (!bo) Here we're missing the drmIoctl(... DRM_IOCTL_GEM_CLOSE ...) Another solution is to move size/pitch calculation before the calloc. Not sure which one will be better though. Emil _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND][PATCH] libkms/exynos: fix memory leak in error path 2016-12-14 12:21 ` Emil Velikov @ 2016-12-14 13:12 ` Eric Engestrom 2016-12-14 17:12 ` Emil Velikov 0 siblings, 1 reply; 6+ messages in thread From: Eric Engestrom @ 2016-12-14 13:12 UTC (permalink / raw) To: Emil Velikov; +Cc: Seung-Woo Kim, Rob Clark, ML dri-devel On Wednesday, 2016-12-14 12:21:55 +0000, Emil Velikov wrote: > On 14 December 2016 at 12:07, Eric Engestrom <eric.engestrom@imgtec.com> wrote: > > On Wednesday, 2016-12-14 17:16:30 +0900, Seung-Woo Kim wrote: > >> This patch fixes memory leak in error path of exynos_bo_create(). > > > > Indeed, thanks! > > Reviewed-by: Eric Engestrom <eric.engestrom@imgtec.com> > > > >> > >> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com> > >> --- > >> libkms/exynos.c | 3 ++- > >> 1 files changed, 2 insertions(+), 1 deletions(-) > >> > >> diff --git a/libkms/exynos.c b/libkms/exynos.c > >> index 5de2e5a..0e97fb5 100644 > >> --- a/libkms/exynos.c > >> +++ b/libkms/exynos.c > >> @@ -88,7 +88,8 @@ exynos_bo_create(struct kms_driver *kms, > >> pitch = (pitch + 512 - 1) & ~(512 - 1); > >> size = pitch * ((height + 4 - 1) & ~(4 - 1)); > >> } else { > >> - return -EINVAL; > >> + ret = -EINVAL; > >> + goto err_free; > >> } > >> > >> memset(&arg, 0, sizeof(arg)); > >> -- > >> 1.7.4.1 > >> > > > > However, I feel like a cleaner fix might be to simply move the > > allocation to where it's used and remove the now-unnecessary > > error path, ie.: > > > > ----8<---- > > diff --git a/libkms/exynos.c b/libkms/exynos.c > > index 5de2e5a..e2c1c9f 100644 > > --- a/libkms/exynos.c > > +++ b/libkms/exynos.c > > @@ -76,10 +76,6 @@ exynos_bo_create(struct kms_driver *kms, > > } > > } > > > > - bo = calloc(1, sizeof(*bo)); > > - if (!bo) > > - return -ENOMEM; > > - > > if (type == KMS_BO_TYPE_CURSOR_64X64_A8R8G8B8) { > > pitch = 64 * 4; > > size = 64 * 64 * 4; > > @@ -96,7 +92,11 @@ exynos_bo_create(struct kms_driver *kms, > > > > ret = drmCommandWriteRead(kms->fd, DRM_EXYNOS_GEM_CREATE, &arg, sizeof(arg)); > > if (ret) > > - goto err_free; > > + return ret; > > + > > + bo = calloc(1, sizeof(*bo)); > > + if (!bo) > Here we're missing the drmIoctl(... DRM_IOCTL_GEM_CLOSE ...) You're right... I'm not actually improving anything with my suggestion. > > Another solution is to move size/pitch calculation before the calloc. > Not sure which one will be better though. Let's just land Seung-Woo's fix? We can always refactor later :) _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND][PATCH] libkms/exynos: fix memory leak in error path 2016-12-14 13:12 ` Eric Engestrom @ 2016-12-14 17:12 ` Emil Velikov 0 siblings, 0 replies; 6+ messages in thread From: Emil Velikov @ 2016-12-14 17:12 UTC (permalink / raw) To: Eric Engestrom; +Cc: Seung-Woo Kim, Rob Clark, ML dri-devel On 14 December 2016 at 13:12, Eric Engestrom <eric.engestrom@imgtec.com> wrote: > On Wednesday, 2016-12-14 12:21:55 +0000, Emil Velikov wrote: >> On 14 December 2016 at 12:07, Eric Engestrom <eric.engestrom@imgtec.com> wrote: >> > On Wednesday, 2016-12-14 17:16:30 +0900, Seung-Woo Kim wrote: >> >> This patch fixes memory leak in error path of exynos_bo_create(). >> > >> > Indeed, thanks! >> > Reviewed-by: Eric Engestrom <eric.engestrom@imgtec.com> >> > >> >> >> >> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com> >> >> --- >> >> libkms/exynos.c | 3 ++- >> >> 1 files changed, 2 insertions(+), 1 deletions(-) >> >> >> >> diff --git a/libkms/exynos.c b/libkms/exynos.c >> >> index 5de2e5a..0e97fb5 100644 >> >> --- a/libkms/exynos.c >> >> +++ b/libkms/exynos.c >> >> @@ -88,7 +88,8 @@ exynos_bo_create(struct kms_driver *kms, >> >> pitch = (pitch + 512 - 1) & ~(512 - 1); >> >> size = pitch * ((height + 4 - 1) & ~(4 - 1)); >> >> } else { >> >> - return -EINVAL; >> >> + ret = -EINVAL; >> >> + goto err_free; >> >> } >> >> >> >> memset(&arg, 0, sizeof(arg)); >> >> -- >> >> 1.7.4.1 >> >> >> > >> > However, I feel like a cleaner fix might be to simply move the >> > allocation to where it's used and remove the now-unnecessary >> > error path, ie.: >> > >> > ----8<---- >> > diff --git a/libkms/exynos.c b/libkms/exynos.c >> > index 5de2e5a..e2c1c9f 100644 >> > --- a/libkms/exynos.c >> > +++ b/libkms/exynos.c >> > @@ -76,10 +76,6 @@ exynos_bo_create(struct kms_driver *kms, >> > } >> > } >> > >> > - bo = calloc(1, sizeof(*bo)); >> > - if (!bo) >> > - return -ENOMEM; >> > - >> > if (type == KMS_BO_TYPE_CURSOR_64X64_A8R8G8B8) { >> > pitch = 64 * 4; >> > size = 64 * 64 * 4; >> > @@ -96,7 +92,11 @@ exynos_bo_create(struct kms_driver *kms, >> > >> > ret = drmCommandWriteRead(kms->fd, DRM_EXYNOS_GEM_CREATE, &arg, sizeof(arg)); >> > if (ret) >> > - goto err_free; >> > + return ret; >> > + >> > + bo = calloc(1, sizeof(*bo)); >> > + if (!bo) >> Here we're missing the drmIoctl(... DRM_IOCTL_GEM_CLOSE ...) > > You're right... I'm not actually improving anything with my suggestion. > >> >> Another solution is to move size/pitch calculation before the calloc. >> Not sure which one will be better though. > > Let's just land Seung-Woo's fix? > We can always refactor later :) Agreed. it was just an idea for the future. Thanks gents, the patch is in master now. -Emil _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-12-14 17:12 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-11-14 5:31 [PATCH libdrm] libkms/exynos: fix memory leak in error path Seung-Woo Kim 2016-12-14 8:16 ` [RESEND][PATCH] " Seung-Woo Kim 2016-12-14 12:07 ` Eric Engestrom 2016-12-14 12:21 ` Emil Velikov 2016-12-14 13:12 ` Eric Engestrom 2016-12-14 17:12 ` Emil Velikov
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.