[SSSD] [PATCHES] fix minor memory leaks

Lukas Slebodnik lslebodn at redhat.com
Mon Sep 21 09:16:30 UTC 2015


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


More information about the sssd-devel mailing list