On Thu, Apr 07, 2016 at 01:01:52PM +0200, Pavel Březina wrote:
> On 04/07/2016 11:51 AM, Jakub Hrozek wrote:
> >On Thu, Apr 07, 2016 at 09:16:21AM +0200, Lukas Slebodnik wrote:
> >>On (06/04/16 18:38), Jakub Hrozek wrote:
> >>>Hi,
> >>>
> >>>I'm sorry I didn't catch this when I developed the original patch, but
> >>>today I was trying to write the leak patches as Lukas suggested the
> >>>other day..I haven't succeeded at that yet, but I found another leak.
> >>>
> >>Even if you will not able to write a test your effort has some outcome.
> >>I glad my idea was not useless.
> >
> >I'm actually not sure how to write them properly. Normally for the leak
> >checks, we would use global_talloc_context instead of NULL in the code
> >from the start, but since the memberof plugin uses NULL internally and
> >it's a plugin, so there's no _send-style entry point, this wouldn't
> >work.
> >
> >I'll try to see if we can use talloc_enable_null_tracking(), though, but
> >ldb also leaks some memory on the NULL context during normal operation
> >(IIRC when loading modules), so maybe we'll have to do some trickery
> >there..
>
> You can use talloc_total_size(NULL) if you enable null tracking.
OK, that helped me find another leak..see patch #2.
But I'm not sure full NULL-tracking is practical in tests, because there
are so many leaks happening in ldb during normal operation that we
always find some. Mostly it's when casefolding DNs or exploding DNs
For example:
==31730== 666 bytes in 6 blocks are possibly lost in loss record 1,132 of 1,425
==31730== at 0x4C28C50: malloc (in
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==31730== by 0x8BEB7E5: __talloc_with_prefix (talloc.c:668)
==31730== by 0x8BEB7E5: __talloc (talloc.c:708)
==31730== by 0x8BEB7E5: _talloc_named_const (talloc.c:865)
==31730== by 0x8BEB7E5: _talloc_array (talloc.c:2587)
==31730== by 0x8E0582B: ldb_val_dup (ldb_msg.c:106)
==31730== by 0x8E0D437: ldb_handler_copy (attrib_handlers.c:39)
==31730== by 0x8E082B2: ldb_dn_casefold_internal (ldb_dn.c:937)
==31730== by 0x8E091B7: ldb_dn_compare (ldb_dn.c:1136)
==31730== by 0xC1BAEA4: mbof_append_muop (memberof.c:246)
==31730== by 0xC1BF812: mbof_del_fill_muop (memberof.c:2484)
==31730== by 0xC1BD6D2: mbof_orig_del_callback (memberof.c:1539)
==31730== by 0xD60353F: ltdb_callback (ldb_tdb.c:1398)
==31730== by 0x89DDA0C: tevent_common_loop_timer_delay (tevent_timed.c:341)
==31730== by 0x89DEA39: epoll_event_loop_once (tevent_epoll.c:911)
or:
==31730== 714 bytes in 7 blocks are possibly lost in loss record 1,152 of 1,425
==31730== at 0x4C28C50: malloc (in
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==31730== by 0x8BEA613: __talloc_with_prefix (talloc.c:668)
==31730== by 0x8BEA613: __talloc (talloc.c:708)
==31730== by 0x8BEA613: _talloc_named_const (talloc.c:865)
==31730== by 0x8BEA613: _talloc_memdup (talloc.c:2286)
==31730== by 0x8E07A3A: ldb_dn_explode.part.1 (ldb_dn.c:713)
==31730== by 0xC1BF787: mbof_del_fill_muop (memberof.c:2477)
==31730== by 0xC1BD6D2: mbof_orig_del_callback (memberof.c:1539)
==31730== by 0xD60353F: ltdb_callback (ldb_tdb.c:1398)
==31730== by 0x89DDA0C: tevent_common_loop_timer_delay (tevent_timed.c:341)
==31730== by 0x89DEA39: epoll_event_loop_once (tevent_epoll.c:911)
==31730== by 0x89DD136: std_event_loop_once (tevent_standard.c:114)
==31730== by 0x89D938C: _tevent_loop_once (tevent.c:533)
==31730== by 0x8E1647E: ldb_wait (ldb.c:631)
==31730== by 0x8E16F7B: ldb_autotransaction_request (ldb.c:573)
So I'm not sure if it's practical to enable NULL tracking. Feel free to
only use the third patch for verification of the previous leak fixes, but
I guess we should only commit the first two.. If we want to have tests,
then I think valgrind suppression would be more practical.
>From c69aff1284c72d1e5bcb633c2499e9fb74f16335 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Wed, 6 Apr 2016 18:35:39 +0200
Subject: [PATCH 1/3] memberof: Fix a memory leak when removing ghost users
---
src/ldb_modules/memberof.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/ldb_modules/memberof.c b/src/ldb_modules/memberof.c
index
54e4b3ee2c74b746e8871cb3bb211bfcb25752e0..0d909bfe811078861f0f0c8cc54c76752397f1c9
100644
--- a/src/ldb_modules/memberof.c
+++ b/src/ldb_modules/memberof.c
@@ -2531,7 +2531,7 @@ static int mbof_del_fill_ghop_ex(struct mbof_del_ctx
*del_ctx,
num_gh_vals, mbof->num_values);
for (i = 0; i < mbof->num_values; i++) {
- valdn = ldb_dn_from_ldb_val(del_ctx->ghops,
+ valdn = ldb_dn_from_ldb_val(del_ctx,
ldb_module_get_ctx(del_ctx->ctx->module),
&mbof->values[i]);
if (!valdn || !ldb_dn_validate(valdn)) {
@@ -2556,6 +2556,7 @@ static int mbof_del_fill_ghop_ex(struct mbof_del_ctx
*del_ctx,
if (ret != LDB_SUCCESS) {
return ret;
}
+ talloc_steal(del_ctx->ghops, valdn);
}
}
--
2.4.11
>From 660413914a9f3061d58acb25a261dcaf3dbbeefe Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Fri, 8 Apr 2016 11:47:44 +0200
Subject: [PATCH 2/3] memberof: Don't allocate on NULL when deleting memberUids
---
src/ldb_modules/memberof.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/ldb_modules/memberof.c b/src/ldb_modules/memberof.c
index
0d909bfe811078861f0f0c8cc54c76752397f1c9..5fda9deb8b95f4a6fcae86d480c6ba00267f9d4b
100644
--- a/src/ldb_modules/memberof.c
+++ b/src/ldb_modules/memberof.c
@@ -2471,7 +2471,7 @@ static int mbof_del_fill_muop(struct mbof_del_ctx
*del_ctx,
for (i = 0; i < el->num_values; i++) {
struct ldb_dn *valdn;
- valdn = ldb_dn_from_ldb_val(del_ctx->muops,
+ valdn = ldb_dn_from_ldb_val(del_ctx,
ldb_module_get_ctx(del_ctx->ctx->module),
&el->values[i]);
if (!valdn || !ldb_dn_validate(valdn)) {
@@ -2489,6 +2489,7 @@ static int mbof_del_fill_muop(struct mbof_del_ctx
*del_ctx,
if (ret != LDB_SUCCESS) {
return ret;
}
+ talloc_steal(del_ctx->muops, valdn);
}
return LDB_SUCCESS;
--
2.4.11
>From 57ac0ce1b085bf5fe3f90b9dfa7a3e8cc769c83a Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Fri, 8 Apr 2016 11:13:00 +0200
Subject: [PATCH 3/3] tests: Check NULL context in sysdb-tests when removing
group members
This is done to make sure the memberof module does not leak memory.
---
src/tests/sysdb-tests.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c
index
d64e31cfbf5d3874a39bcac8b11b010aa67b04b2..3c3e30a6458cda6624ad60e6f05bdca3fe6c1684
100644
--- a/src/tests/sysdb-tests.c
+++ b/src/tests/sysdb-tests.c
@@ -59,6 +59,8 @@ struct sysdb_test_ctx {
struct confdb_ctx *confdb;
struct tevent_context *ev;
struct sss_domain_info *domain;
+
+ size_t null_pointer_size;
};
static int _setup_sysdb_tests(struct sysdb_test_ctx **ctx, bool enumerate)
@@ -155,10 +157,28 @@ static int _setup_sysdb_tests(struct sysdb_test_ctx
**ctx, bool enumerate)
}
test_ctx->sysdb = test_ctx->domain->sysdb;
+ test_ctx->null_pointer_size = talloc_total_size(NULL);
+
*ctx = test_ctx;
return EOK;
}
+static void null_ctx_get_size(struct sysdb_test_ctx *ctx)
+{
+ ctx->null_pointer_size = talloc_total_size(NULL);
+}
+
+static void fail_if_null_ctx_leaks(struct sysdb_test_ctx *ctx)
+{
+ size_t new_null_pointer_size;
+
+ new_null_pointer_size = talloc_total_size(NULL);
+ if(new_null_pointer_size != ctx->null_pointer_size) {
+ fail("NULL pointer leaked memory, was %zu, is %zu\n",
+ ctx->null_pointer_size, new_null_pointer_size);
+ }
+}
+
#define setup_sysdb_tests(ctx) _setup_sysdb_tests((ctx), false)
struct test_data {
@@ -743,7 +763,9 @@ START_TEST (test_sysdb_remove_local_group_by_gid)
data->ev = test_ctx->ev;
data->gid = _i;
+ null_ctx_get_size(data->ctx);
ret = test_remove_group_by_gid(data);
+ fail_if_null_ctx_leaks(test_ctx);
fail_if(ret != EOK, "Could not remove group with gid %d", _i);
talloc_free(test_ctx);
@@ -3160,8 +3182,10 @@ START_TEST (test_sysdb_memberof_mod_del)
}
/* Delete the attribute */
+ null_ctx_get_size(test_ctx);
ret = sysdb_set_group_attr(test_ctx->domain,
data->groupname, data->attrs, SYSDB_MOD_DEL);
+ fail_if_null_ctx_leaks(test_ctx);
fail_unless(ret == EOK, "Cannot set group attrs\n");
/* After the delete, we shouldn't be able to find the ghost attribute */
@@ -6800,6 +6824,7 @@ int main(int argc, const char *argv[]) {
}
tests_set_cwd();
+ talloc_enable_null_tracking();
test_dom_suite_cleanup(TESTS_PATH, TEST_CONF_FILE, LOCAL_SYSDB_FILE);
--
2.4.11
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
