On 03/22/2016 11:05 AM, Lukas Slebodnik wrote:
On (22/03/16 10:37), Pavel Březina wrote:
> On 03/21/2016 02:06 PM, Lukas Slebodnik wrote:
>> On (21/03/16 13:51), Pavel Březina wrote:
>>> On 03/21/2016 01:10 PM, Lukas Slebodnik wrote:
>>>> On (21/03/16 11:02), Petr Cech wrote:
>>>>> On 03/16/2016 12:37 PM, Pavel Březina wrote:
>>>>>
>>>>> [...]
>>>>>
>>>>>> *Patch 2: SYSDB: Add new funtions into sysdb_sudo*
>>>>>>
>>>>>>> +errno_t
>>>>>>> +sysdb_set_sudo_rule_attr(struct sss_domain_info *domain,
>>>>>>> + const char *name,
>>>>>>> + struct sysdb_attrs *attrs,
>>>>>>> + int mod_op)
>>>>>>> +{
>>>>>>> + errno_t ret;
>>>>>>> + struct ldb_dn *dn;
>>>>>>> + TALLOC_CTX *tmp_ctx;
>>>>>>> +
>>>>>>> + tmp_ctx = talloc_new(NULL);
>>>>>>> + if (tmp_ctx == NULL) {
>>>>>>> + return ENOMEM;
>>>>>>> + }
>>>>>>> +
>>>>>>> + dn = sysdb_sudo_rule_dn(tmp_ctx, domain, name);
>>>>>>> + NULL_CHECK(dn, ret, done);
>>>>>>> +
>>>>>>> + ret = sysdb_set_entry_attr(domain->sysdb, dn, attrs,
mod_op);
>>>>>>> +
>>>>>>> +done:
>>>>>>> + talloc_free(tmp_ctx);
>>>>>>> + return ret;
>>>>>>>>> Temporary context is not needed here, you can use
NULL instead and
>>>>>>>>> talloc_free(dn) in the end.
>>>>>>>>
>>>>>>>> I am not sure about it. I have tried it but it didn't
work for me.
>>>>>>>>
>>>>>>
>>>>>> What didn't work for you?
>>>>>>
>>>>>> errno_t
>>>>>> sysdb_set_sudo_rule_attr(struct sss_domain_info *domain,
>>>>>> const char *name,
>>>>>> struct sysdb_attrs *attrs,
>>>>>> int mod_op)
>>>>>> {
>>>>>> struct ldb_dn *dn;
>>>>>> errno_t ret;
>>>>>>
>>>>>> dn = sysdb_sudo_rule_dn(NULL, domain, name);
>>>>>> if (dn == NULL) {
>>>>>> return ENOMEM;
>>>>>> }
>>>>>>
>>>>>> ret = sysdb_set_entry_attr(domain->sysdb, dn, attrs,
mod_op);
>>>>>> talloc_free(dn);
>>>>>> return ret;
>>>>>> }
>>>>>
>>>>> Hi list,
>>>>>
>>>>> I think that I find bug in ldb-1.1.25:
>>>>>
>>>>>
>>>>> So, I have done little investigation.
>>>>>
>>>>> In short, backtrace in gdb says:
>>>>>
>>>>> #0 ldb_dn_new_fmt (mem_ctx=0x0, ldb=0x6578f0, new_fmt=0x7ffff79b3538
>>>>> "name=%s,cn=%s,cn=custom,cn=%s,cn=sysdb") at
../common/ldb_dn.c:174
>>>>> #1 0x00007ffff796727c in sysdb_custom_dn (mem_ctx=0x0, dom=0x658500,
>>>>> object_name=0x409f34 "test_rule3", subtree_name=0x40aecf
"sudorules") at
>>>>> ../src/db/sysdb.c:148
>>>>> #2 0x00000000004073e8 in sysdb_sudo_rule_dn (mem_ctx=0x0,
domain=0x658500,
>>>>> name=0x409f34 "test_rule3") at ../src/db/sysdb_sudo.c:962
>>>>> #3 0x0000000000407416 in sysdb_set_sudo_rule_attr (domain=0x658500,
>>>>> name=0x409f34 "test_rule3", attrs=0x658390, mod_op=2) at
>>>>> ../src/db/sysdb_sudo.c:974
>>>>> #4 0x000000000040458b in test_set_sudo_rule_attr_exist
(state=0x614a90) at
>>>>> ../src/tests/cmocka/test_sysdb_sudo.c:400
>>>>> #5 0x00007ffff7bd6659 in cmocka_run_one_test_or_fixture
>>>>> (function_name=0x40a8e9 "test_set_sudo_rule_attr_exist",
test_func=0x4044d7
>>>>> <test_set_sudo_rule_attr_exist>,
setup_func=setup_func@entry=0x0,
>>>>> teardown_func=teardown_func@entry=0x0, state=state@entry=0x614a90,
>>>>> heap_check_point=heap_check_point@entry=0x0) at
>>>>> /usr/src/debug/cmocka-1.0.1/src/cmocka.c:2305
>>>>> #6 0x00007ffff7bd6d8f in cmocka_run_one_tests (test_state=0x614a80)
at
>>>>> /usr/src/debug/cmocka-1.0.1/src/cmocka.c:2413
>>>>> #7 _cmocka_run_group_tests (group_name=0x40a823 "tests",
tests=<optimized
>>>>> out>, num_tests=15, group_setup=<optimized out>,
group_teardown=0x0) at
>>>>> /usr/src/debug/cmocka-1.0.1/src/cmocka.c:2518
>>>>> #8 0x0000000000404d7a in main (argc=1, argv=0x7fffffffe3f8) at
>>>>> ../src/tests/cmocka/test_sysdb_sudo.c:645
>>>>>
>>>>>
>>>>> On the line
>>>>> /usr/src/debug/ldb-1.1.25/common/ldb_dn.c:174:
>>>>>
>>>>> 167 struct ldb_dn *ldb_dn_new_fmt(TALLOC_CTX *mem_ctx,
>>>>> 168 struct ldb_context *ldb,
>>>>> 169 const char *new_fmt, ...)
>>>>> 170 {
>>>>> 171 char *strdn;
>>>>> 172 va_list ap;
>>>>> 173
>>>>> 174 if ( (! mem_ctx) || (! ldb)) return NULL;
>>>>> 175
>>>>> 176 va_start(ap, new_fmt);
>>>>> 177 strdn = talloc_vasprintf(mem_ctx, new_fmt, ap);
>>>>> 178 va_end(ap);
>>>>> 179
>>>>> 180 if (strdn) {
>>>>> 181 struct ldb_dn *dn = ldb_dn_new(mem_ctx, ldb, strdn);
>>>>> 182 talloc_free(strdn);
>>>>> 183 return dn;
>>>>> 184 }
>>>>> 185
>>>>> 186 return NULL;
>>>>> 187 }
>>>>> 188
>>>>>
>>>>> How you can see, if we send NULL to sysdb_sudo_rule_dn, it will
explode very
>>>>> badly :-)
>>>>>
>>>> It might be a bug. But we should try to avoid using NULL context
>>>>
>>>> We had some memory leaks caused by allocating something on NULL context.
>>>>
>>>> LS
>>>
>>> Except in this case this is the same thing as using temporary context but
>>> shorter.
>> If you want to avoid creating tmp_ctx you can still use "ldb"
>> as talloc context and you will not hit bug in ldb_dn_new_fmt.
>
> I agree with you with this as a workaround to avoid creating new talloc
> context here.
>
>>
>> If you extend this function and possibly create memory leak
>> it will be easier to find it.
>
> Actually no, finding memory leaked on NULL context is easier than finding it
> on existing talloc context. Nothing but top level context is supposed to be
> attached to NULL context so if there is anything else we have a leak, it's
> simple as that. Using existing talloc context makes it harder to find and it
> may not even show because you free it somewhere in the process.
It needn't be simpler.
Why?
some_ctx = NULL;
fun1(some_ctx, ....) {
fun2(some_ctx, ...);
}
and you leak some memory in fun2.
How simple would be find such memory leak?
One thing is to find location of the leak, another thing is to detect
that a leak exist and what data are we leaking. I am not going to spend
more time on this discussion, there are some common practices that we
use and using NULL parent for temporary context is one of them. I'll
leave it entirely up to you if you want to follow them or not (until
I'll review your code where we can discuss pros and cons in a particular
situations. And please note that I do value your opinion even though I
don't always agree and sometimes you are able to give me another
perspective that I don't see but I would welcome if you stop creating
long and time consuming discussions on trivial things unless you want to
fully participate in the review process.
BTW this is exactly recently fixed leak in memberof plugin.