[SSSD] [PATCHES] fix minor memory leaks

Jakub Hrozek jhrozek at redhat.com
Tue Sep 29 15:49:32 UTC 2015


On Tue, Sep 29, 2015 at 02:20:39PM +0200, Pavel Reichl wrote:
> 
> 
> On 09/29/2015 02:01 PM, Lukas Slebodnik wrote:
> >On (29/09/15 13:32), Pavel Reichl wrote:
> >>
> >>
> >>On 09/21/2015 11:16 AM, Lukas Slebodnik wrote:
> >>>On (21/09/15 10:59), Pavel Březina wrote:
> >>>>On 09/21/2015 10:58 AM, Lukas Slebodnik wrote:
> >>>>>On (21/09/15 10:40), Pavel Březina wrote:
> >>>>>>On 09/20/2015 07:58 PM, Pavel Reichl wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>>On 09/04/2015 01:56 PM, Jakub Hrozek wrote:
> >>>>>>>>On Fri, Sep 04, 2015 at 01:09:36PM +0200, Pavel Reichl wrote:
> >>>>>>>>>Hello,
> >>>>>>>>>
> >>>>>>>>>please see simple patch set fixing minor memory leaks of providers.
> >>>>>>>>>I'm not aware of any user hitting those currently.
> >>>>>>>>>
> >>>>>>>>>Thanks!
> >>>>>>>>
> >>>>>>>>> From ef62f0245cde314fd11b1cd2584589e018ede050 Mon Sep 17 00:00:00 2001
> >>>>>>>>>From: Pavel Reichl <preichl at redhat.com>
> >>>>>>>>>Date: Fri, 4 Sep 2015 07:02:42 -0400
> >>>>>>>>>Subject: [PATCH 1/4] AD: fix minor memory leak
> >>>>>>>>>
> >>>>>>>>>---
> >>>>>>>>>  src/providers/ad/ad_common.c | 7 ++++---
> >>>>>>>>>  1 file changed, 4 insertions(+), 3 deletions(-)
> >>>>>>>>>
> >>>>>>>>>diff --git a/src/providers/ad/ad_common.c b/src/providers/ad/ad_common.c
> >>>>>>>>>index
> >>>>>>>>>130cdeb613aae3843f7453a478815daaae6aab77..aca550581708e6501c99f1c69e3c5ec9303d3b8c
> >>>>>>>>>100644
> >>>>>>>>>--- a/src/providers/ad/ad_common.c
> >>>>>>>>>+++ b/src/providers/ad/ad_common.c
> >>>>>>>>>@@ -658,7 +658,7 @@ ad_failover_init(TALLOC_CTX *mem_ctx, struct
> >>>>>>>>>be_ctx *bectx,
> >>>>>>>>>      TALLOC_CTX *tmp_ctx;
> >>>>>>>>>      struct ad_service *service;
> >>>>>>>>>
> >>>>>>>>>-    tmp_ctx = talloc_new(mem_ctx);
> >>>>>>>>>+    tmp_ctx = talloc_new(NULL);
> >>>>>>>>
> >>>>>>>>I don't think we should do this kind of change, it's quite possible
> >>>>>>>>we'll start using talloc pools soon which will make us migrate from
> >>>>>>>>using NULL context.
> >>>>>>>>
> >>>>>>>>Just make sure tmp_ctx is freed as appropriate.
> >>>>>>>>
> >>>>>>>>I haven't reviewed the rest of the patches.
> >>>>>>>>
> >>>>>>>>>      if (!tmp_ctx) return ENOMEM;
> >>>>>>>>
> >>>>>>>Bump. (I think that the first patch nacked by Jakub can be ignored.)
> >>>>>>
> >>>>>>Ack to all.
> >>>>>>
> >>>>>>I think even the first one should be pushed. 1) I don't think we are going to
> >>>>>>use talloc pools anytime soon. 2) When and if we are going to use them it
> >>>>>>will be a big change and in my opinion it will be more doable to convert
> >>>>>>consistent code.
> >>>>>>
> >>>>>Consistent code does not require following change in the 1st patch
> >>>>>-    tmp_ctx = talloc_new(mem_ctx);
> >>>>>+    tmp_ctx = talloc_new(NULL);
> >>>>>
> >>>>>temporary context needn't be allocated on NULL context
> >>>>>IMHO; it can be allocated on mem_ctx without any issue.
> >>>>
> >>>>It can. It is not our coding style though.
> >>>>
> >>>But it needn't :-)
> >>>So this change is not necessary.
> >>>
> >>>I checked the source code and in 10% of cases we do not use
> >>>NULL context for temporary context. Moreover if we want to use talloc pools
> >>>in future then it would be better to reduce usage of NULL context.
> >>>
> >>>It's fine to use NULL context for allocation in function which can be unit
> >>>tested (leak_check_setup, leak_check_teardown); but it is not this case
> >>>in the 1st patch.
> >>>
> >>>LS
> >>>
> >>
> >>As the discussion about the first patch stalled I propose to omit it and push the rest of the patch set to master.
> >>
> >>Should I send updated patch set or committer can skip the first patch himself?
> >It would be good to to also push 1st patch
> >without change
> >-    tmp_ctx = talloc_new(mem_ctx);
> >+    tmp_ctx = talloc_new(NULL);
> >
> 
> OK, I'll amend the patch.
> 
> >
> >There were two -1 for the first patch (jhrozek, lslebodn)
> >and one +1 from pbrezina.
> >
> >If we consider you an author then there is another +1.
> >
> >Do we need another (5th person) to decide which way is better?
> >or are two -1s enough to remove such change?
> 
> In my opinion we can drop the change. This is not an imminent bug it's rather code style dispute and I don't think that we have to bother Sumit for that. Unless of course pbrezina feels different.

I don't mind too much, it just doesn't seem to be helpful change, that's
all, so I'll be a bit happier if we drop it.

On the other hand, if we decide to use talloc pools, then we'll change
all or most NULL contexts, which would essentially revert this change.


More information about the sssd-devel mailing list