[SSSD] [PATCHES] fix minor memory leaks

Lukas Slebodnik lslebodn at redhat.com
Tue Sep 29 12:01:49 UTC 2015


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


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?

LS


More information about the sssd-devel mailing list