#407 dna memleak reported by valgrind
Closed: Fixed None Opened 7 years ago by rmeggins.

Running the multi_plugin test in valgrind:
==27981== 24 bytes in 1 blocks are definitely lost in loss record 248 of 1,456
==27981== at 0x4A04A28: calloc (vg_replace_malloc.c:467)
==27981== by 0x4C57EE9: slapi_ch_calloc (ch_malloc.c:243)
==27981== by 0x4CA11AC: slapi_mods_new (modutil.c:93)
==27981== by 0xB0FAE6B: dna_pre_op (dna.c:3254)
==27981== by 0xB0FB08C: dna_mod_pre_op (dna.c:3334)
==27981== by 0x4CB2228: plugin_call_func (plugin.c:1453)
==27981== by 0x4CB20DB: plugin_call_list (plugin.c:1415)
==27981== by 0x4CB066F: plugin_call_plugins (plugin.c:398)
==27981== by 0xB577B8B: ldbm_back_modify (ldbm_modify.c:442)
==27981== by 0x4C9E6B6: op_shared_modify (modify.c:945)
==27981== by 0x4C9D8E2: modify_internal_pb (modify.c:616)
==27981== by 0x4C9D45E: slapi_modify_internal_pb (modify.c:471)
==27981== by 0x4CBF4F3: pw_mod_allowchange_aci (pw.c:1324)
==27981== by 0x4291ED: pw_init (pw_mgmt.c:304)
==27981== by 0x42150F: main (main.c:1217)


Sending fix out for review...

It's not that simple. The problem is that you cannot just call slapi_mods_free(&smods) in every case.
{{{
slapi_pblock_get(pb, SLAPI_MODIFY_MODS, &mods);
smods = slapi_mods_new();
slapi_mods_init_passin(smods, mods);
}}}
once this happens, smods owns mods. This means when you call slapi_mods_free(&smods) it will free the mods as well, since it owns that memory.

I have new patch (attached) but I can't verify it without a testcase. I would think its ok, as I just removed the allocation all together, and used a local variable.

Could still leak memory:
{{{
if (LDAP_CHANGETYPE_ADD == modtype) {
ret = _dna_pre_op_add(pb, test_e);
} else {
- ret = _dna_pre_op_modify(pb, test_e, smods);
+ ret = _dna_pre_op_modify(pb, test_e, &smods);
}
if (ret) {
goto bail;
}}}
If ret is set and it does the goto bail, if smods has been changed, those changes will be leaked. Also, it's not a good idea to use the structure directly - better to stick with the slapi API.

Ok, here is patch number 3...

Thanks for the review Rich!

git merge ticket407
Updating 657efc6..9229db2
Fast-forward
ldap/servers/plugins/dna/dna.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)

git push origin master
Counting objects: 13, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (7/7), done.
Writing objects: 100% (7/7), 854 bytes, done.
Total 7 (delta 5), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
657efc6..9229db2 master -> master

[mareynol@localhost ds]$ git merge ticket407
Updating 52e1507..60316fc
Fast-forward
ldap/servers/plugins/dna/dna.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

[mareynol@localhost ds]$ git push origin master
Counting objects: 13, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (7/7), done.
Writing objects: 100% (7/7), 682 bytes, done.
Total 7 (delta 5), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
52e1507..60316fc master -> master

Added initial screened field value.

Metadata Update from @nkinder:
- Issue assigned to mreynolds
- Issue set to the milestone: 1.2.11.8

3 years ago

Login to comment on this ticket.

Metadata