IDMAP: Fix minor memory leak

[Pavel Reichl]

Hello,

please see simple patch attached.

Memory leak is a minor problem as code is called just once for a new domain and it would occur only if run out of memory or all slices.

Thanks

[Michal Židek]

On 01/22/2016 06:38 PM, Pavel Reichl wrote:
> Hello,
>
> please see simple patch attached.
>
> Memory leak is a minor problem as code is called just once for a new
> domain and it would occur only if run out of memory or all slices.
>
> Thanks
>
> 0001-IDMAP-Fix-minor-memory-leak.patch
>
> From 0368027c4b8eb0b4ffb4256332ace9a51133cfde Mon Sep 17 00:00:00 2001
> From: Pavel Reichl
> Date: Fri, 22 Jan 2016 12:30:23 -0500
> Subject: [PATCH] IDMAP: Fix minor memory leak
>
> src/lib/idmap/sss_idmap.c | 14 +++++++++++---
>   1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/src/lib/idmap/sss_idmap.c b/src/lib/idmap/sss_idmap.c
> index 269ef0132ff3b9ffbfbe65006361fac6d4f88cf9..41a9f92724913c2311a83668f2f56dfe292ed8fe 100644
> --- a/src/lib/idmap/sss_idmap.c
> +++ b/src/lib/idmap/sss_idmap.c
> @@ -607,13 +607,13 @@ get_helpers(struct sss_idmap_ctx *ctx,
>       for (int i = 0; i < ctx->idmap_opts.extra_slice_init; i++) {
>           secondary_name = generate_sec_slice_name(ctx, domain_sid, first_rid);
>           if (secondary_name == NULL) {
>        return IDMAP_OUT_OF_MEMORY;
>        err = IDMAP_OUT_OF_MEMORY;
>        goto done;
>     }
>
>     err = generate_slice(ctx, secondary_name, first_rid, &slice);
>     if (err != IDMAP_SUCCESS) {
>        ctx->free_func(secondary_name, ctx->alloc_pvt);
>        return err;
>        goto done;
>     }
>
>     first_rid += ctx->idmap_opts.rangesize;
>
> @@ -631,6 +631,14 @@ get_helpers(struct sss_idmap_ctx *ctx,
>   *_sec_slices = sec_slices;
>   return IDMAP_SUCCESS;
>
> +done:

You use this goto target only case of failure. Could you
change it's name to 'fail'? Or alternatively you can refactor
the function to have single exit point and keep the done label.
Choice is yours.
> ctx->free_func(secondary_name, ctx->alloc_pvt);
>
> /* Free already generated helpers. */
>
> free_helpers(ctx, sec_slices, true);
>
> return err;
> }
>
> enum idmap_error_code sss_idmap_add_domain_ex(struct sss_idmap_ctx *ctx,
>
> -- 2.4.3

[Pavel Reichl]

On 01/27/2016 02:58 PM, Michal Židek wrote:
> On 01/22/2016 06:38 PM, Pavel Reichl wrote:

[snip]
> >       first_rid += ctx->idmap_opts.rangesize;
> >
> > @@ -631,6 +631,14 @@ get_helpers(struct sss_idmap_ctx *ctx,
> >   *_sec_slices = sec_slices;
> >   return IDMAP_SUCCESS;
> >
> > +done:
>
> You use this goto target only case of failure. Could you
> change it's name to 'fail'? Or alternatively you can refactor
> the function to have single exit point and keep the done label.
> Choice is yours.

OK, done.

[Michal Židek]

On 01/27/2016 05:11 PM, Pavel Reichl wrote:
> On 01/27/2016 02:58 PM, Michal Židek wrote:
> > On 01/22/2016 06:38 PM, Pavel Reichl wrote:
>
> [snip]
> > >       first_rid += ctx->idmap_opts.rangesize;
> > >
> > > @@ -631,6 +631,14 @@ get_helpers(struct sss_idmap_ctx *ctx,
> > >   *_sec_slices = sec_slices;
> > >   return IDMAP_SUCCESS;
> > >
> > > +done:
> >
> > You use this goto target only case of failure. Could you
> > change it's name to 'fail'? Or alternatively you can refactor
> > the function to have single exit point and keep the done label.
> > Choice is yours.
>
> OK, done.

Thanks.

ACK.

Link to CI:
http://sssd-ci.duckdns.org/logs/job/36/70/summary.html

Michal

[Lukas Slebodnik]

On (28/01/16 13:46), Michal Židek wrote:
> On 01/27/2016 05:11 PM, Pavel Reichl wrote:
> > On 01/27/2016 02:58 PM, Michal Židek wrote:
> > > On 01/22/2016 06:38 PM, Pavel Reichl wrote:
> >
> > [snip]
> > > >      first_rid += ctx->idmap_opts.rangesize;
> > > >
> > > > @@ -631,6 +631,14 @@ get_helpers(struct sss_idmap_ctx *ctx,
> > > >  *_sec_slices = sec_slices;
> > > >  return IDMAP_SUCCESS;
> > > >
> > > > +done:
> > >
> > > You use this goto target only case of failure. Could you
> > > change it's name to 'fail'? Or alternatively you can refactor
> > > the function to have single exit point and keep the done label.
> > > Choice is yours.
> >
> > OK, done.
>
> Thanks.
>
> ACK.
>
> Link to CI:
> http://sssd-ci.duckdns.org/logs/job/36/70/summary.html

It's not big problem because we use talloc with libsss_idmap
so memory would be released even in case of ENOMEM.

master:
* 5554a2a679f72f19f266d660a5681e3b0c657379

and it would be good to keed code of libraries the same
also in stable branch.

sssd-1-13:
* fe8d58c75da2b9b3704bb2ae19f8014323797757

LS