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.