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.
If you extend this function and possibly create memory leak
it will be easier to find it.
LS