diff mbox

[RESEND] libkms/exynos: fix memory leak in error path

Message ID 20161214120742.GH29253@imgtec.com (mailing list archive)
State New, archived
Headers show

Commit Message

Eric Engestrom Dec. 14, 2016, 12:07 p.m. UTC 
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<----
---->8----

Bigger change, but cleaner code IMHO.
What do you think?

Cheers,
  Eric

Comments

Emil Velikov Dec. 14, 2016, 12:21 p.m. UTC | #1 
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
Eric Engestrom Dec. 14, 2016, 1:12 p.m. UTC | #2 
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 :)
Emil Velikov Dec. 14, 2016, 5:12 p.m. UTC | #3 
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
diff mbox

Patch

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