[SSSD] [PATCHES] UTIL: Fix memory leak in switch_creds
Lukas Slebodnik
lslebodn at redhat.com
Mon Nov 9 07:06:02 UTC 2015
ehlo,
You can see a leak in talloc report.
But it was ignored. So we didn't notice it for long time.
http://sssd-ci.duckdns.org/logs/job/29/90/rhel7/ci-build-debug/src/tests/cwrap/become_user-tests.log
The first patch fixes the leak and the last one is prevention
for such mistakes in future.
LS
-------------- next part --------------
>From 219d1bdd378f0a8c17a508e1f3e29a2d5435f4d0 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Sat, 24 Oct 2015 14:19:11 +0200
Subject: [PATCH 1/5] UTIL: Fix memory leak in switch_creds
If we are already requested used then we needn't to call
setreeuid(), setresgid(). But we forgot to relase local
struct sss_creds *ssc, which is used for returnig saved credentials.
---
src/util/become_user.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/util/become_user.c b/src/util/become_user.c
index 7dd2c752b1d0f289e7d82feee6d93e5974823f95..c5e1439b690451699405226c191f9c4eb0f77a8a 100644
--- a/src/util/become_user.c
+++ b/src/util/become_user.c
@@ -150,6 +150,7 @@ errno_t switch_creds(TALLOC_CTX *mem_ctx,
if (myuid == uid && mygid == gid) {
DEBUG(SSSDBG_FUNC_DATA, "Already user [%"SPRIuid"].\n", uid);
+ talloc_zfree(ssc);
return EOK;
}
--
2.5.0
-------------- next part --------------
>From 318b862f473daf9606bf7752283a63b36934908b Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Sat, 24 Oct 2015 15:39:21 +0200
Subject: [PATCH 2/5] TESTS: Initialize leak check
If leak_check_setup is not called then global_talloc_context
was not initialized and check_leaks_pop(global_talloc_context) will fail.
---
src/tests/cmocka/test_child_common.c | 3 +++
src/tests/cwrap/test_become_user.c | 3 +++
2 files changed, 6 insertions(+)
diff --git a/src/tests/cmocka/test_child_common.c b/src/tests/cmocka/test_child_common.c
index 1a62a9099dffcb56572ef7023238ce196bb5741e..f8fd762fee5e8c0a39ea526835631f736e2dfc2d 100644
--- a/src/tests/cmocka/test_child_common.c
+++ b/src/tests/cmocka/test_child_common.c
@@ -45,6 +45,8 @@ static int child_test_setup(void **state)
struct child_test_ctx *child_tctx;
errno_t ret;
+ assert_true(leak_check_setup());
+
check_leaks_push(global_talloc_context);
child_tctx = talloc(global_talloc_context, struct child_test_ctx);
assert_non_null(child_tctx);
@@ -75,6 +77,7 @@ static int child_test_teardown(void **state)
talloc_free(child_tctx);
check_leaks_pop(global_talloc_context);
+ assert_true(leak_check_teardown());
return 0;
}
diff --git a/src/tests/cwrap/test_become_user.c b/src/tests/cwrap/test_become_user.c
index b9e8a5c3bf216570292b2fbae25832268c6bf17b..48e16d4c75351cf90c64503f377b591038f549e2 100644
--- a/src/tests/cwrap/test_become_user.c
+++ b/src/tests/cwrap/test_become_user.c
@@ -78,6 +78,8 @@ void test_switch_user(void **state)
struct sss_creds *saved_creds;
struct sss_creds *saved_creds2 = NULL;
+ assert_true(leak_check_setup());
+
check_leaks_push(global_talloc_context);
tmp_ctx = talloc_new(global_talloc_context);
assert_non_null(tmp_ctx);
@@ -121,6 +123,7 @@ void test_switch_user(void **state)
check_leaks_pop(tmp_ctx);
talloc_free(tmp_ctx);
check_leaks_pop(global_talloc_context);
+ assert_true(leak_check_teardown());
}
int main(int argc, const char *argv[])
--
2.5.0
-------------- next part --------------
>From 21bf7449bb53209ad24c0ec4079a5810bb6f707b Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Sat, 24 Oct 2015 15:15:39 +0200
Subject: [PATCH 3/5] TESTS: Check return value of check_leaks_pop
---
src/tests/cmocka/test_child_common.c | 3 ++-
src/tests/cwrap/test_become_user.c | 5 +++--
src/tests/cwrap/test_responder_common.c | 6 +++---
src/tests/krb5_utils-tests.c | 6 +++---
src/tests/leak_check.c | 6 +++++-
5 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/src/tests/cmocka/test_child_common.c b/src/tests/cmocka/test_child_common.c
index f8fd762fee5e8c0a39ea526835631f736e2dfc2d..bf500fa5a1f2b2fe79833e23a53cdf0b06b81260 100644
--- a/src/tests/cmocka/test_child_common.c
+++ b/src/tests/cmocka/test_child_common.c
@@ -76,7 +76,8 @@ static int child_test_teardown(void **state)
struct child_test_ctx);
talloc_free(child_tctx);
- check_leaks_pop(global_talloc_context);
+
+ assert_true(check_leaks_pop(global_talloc_context));
assert_true(leak_check_teardown());
return 0;
}
diff --git a/src/tests/cwrap/test_become_user.c b/src/tests/cwrap/test_become_user.c
index 48e16d4c75351cf90c64503f377b591038f549e2..feb6889521d8af47eb0ef563c6bd6f1801b3619f 100644
--- a/src/tests/cwrap/test_become_user.c
+++ b/src/tests/cwrap/test_become_user.c
@@ -120,9 +120,10 @@ void test_switch_user(void **state)
assert_int_equal(getgid(), 0);
talloc_free(saved_creds);
- check_leaks_pop(tmp_ctx);
+ assert_true(check_leaks_pop(tmp_ctx));
talloc_free(tmp_ctx);
- check_leaks_pop(global_talloc_context);
+
+ assert_true(check_leaks_pop(global_talloc_context));
assert_true(leak_check_teardown());
}
diff --git a/src/tests/cwrap/test_responder_common.c b/src/tests/cwrap/test_responder_common.c
index 2c92ff080e28151bc68e52fa02420c331d2ed4cf..024065bc10f7fff52b4a9df0e9581dafb26e786e 100644
--- a/src/tests/cwrap/test_responder_common.c
+++ b/src/tests/cwrap/test_responder_common.c
@@ -60,7 +60,7 @@ void test_uid_csv_to_uid_list(void **state)
assert_int_equal(list[2], 3);
talloc_free(list);
- check_leaks_pop(tmp_ctx);
+ assert_true(check_leaks_pop(tmp_ctx));
talloc_free(tmp_ctx);
}
@@ -83,7 +83,7 @@ void test_name_csv_to_uid_list(void **state)
assert_int_equal(list[1], 10001);
talloc_free(list);
- check_leaks_pop(tmp_ctx);
+ assert_true(check_leaks_pop(tmp_ctx));
talloc_free(tmp_ctx);
}
@@ -102,7 +102,7 @@ void test_csv_to_uid_list_neg(void **state)
ret = csv_string_to_uid_array(tmp_ctx, "nosuchuser", true, &count, &list);
assert_int_not_equal(ret, EOK);
- check_leaks_pop(tmp_ctx);
+ assert_true(check_leaks_pop(tmp_ctx));
talloc_free(tmp_ctx);
}
diff --git a/src/tests/krb5_utils-tests.c b/src/tests/krb5_utils-tests.c
index c2db5af9a2ee50de525ed56481293dc3784a6ec2..ea40feb4584749d01c4d3fb07ab34a93a80a8ab3 100644
--- a/src/tests/krb5_utils-tests.c
+++ b/src/tests/krb5_utils-tests.c
@@ -634,7 +634,7 @@ START_TEST(test_parse_krb5_map_user)
name_to_primary[0].krb_primary == NULL);
talloc_free(name_to_primary);
- check_leaks_pop(mem_ctx);
+ fail_unless(check_leaks_pop(mem_ctx));
}
/* valid input */
{
@@ -653,7 +653,7 @@ START_TEST(test_parse_krb5_map_user)
compare_map_id_name_to_krb_primary(name_to_primary, expected,
sizeof(expected)/sizeof(const char*)/2);
talloc_free(name_to_primary);
- check_leaks_pop(mem_ctx);
+ fail_unless(check_leaks_pop(mem_ctx));
}
/* invalid input */
{
@@ -677,7 +677,7 @@ START_TEST(test_parse_krb5_map_user)
ret = parse_krb5_map_user(mem_ctx, "joe:j:user", &name_to_primary);
fail_unless(ret == EINVAL);
- check_leaks_pop(mem_ctx);
+ fail_unless(check_leaks_pop(mem_ctx));
}
talloc_free(mem_ctx);
diff --git a/src/tests/leak_check.c b/src/tests/leak_check.c
index 1eaa2cd097bda54094098a2947bb3834a9bf04f4..79c8f05f6a3b9a1893d442af4aa67510f1e7a957 100644
--- a/src/tests/leak_check.c
+++ b/src/tests/leak_check.c
@@ -131,7 +131,11 @@ bool
leak_check_teardown(void)
{
bool res;
- check_leaks_pop(global_talloc_context);
+ res = check_leaks_pop(global_talloc_context);
+ if (!res) {
+ _set_leak_err_msg("check_leaks_pop failed in leak_check_teardown");
+ }
+
if (snapshot_stack != NULL) {
_set_leak_err_msg("Exiting with a non-empty stack");
return false;
--
2.5.0
-------------- next part --------------
>From 7e95820146c58e68d9cdf356198d18a3f748ff81 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Fri, 6 Nov 2015 15:13:29 +0100
Subject: [PATCH 4/5] TESTS: Make check_leaks static function
---
src/tests/common.h | 5 -----
src/tests/leak_check.c | 8 ++++----
2 files changed, 4 insertions(+), 9 deletions(-)
diff --git a/src/tests/common.h b/src/tests/common.h
index c0ceebfbf5d9277a422065fbe4ddfdd65c758de8..ac9ba40263fb9d253157c7d07c90dbba04484a74 100644
--- a/src/tests/common.h
+++ b/src/tests/common.h
@@ -34,11 +34,6 @@
extern TALLOC_CTX *global_talloc_context;
-#define check_leaks(ctx, bytes) _check_leaks((ctx), (bytes), __location__)
-bool _check_leaks(TALLOC_CTX *ctx,
- size_t bytes,
- const char *location);
-
void check_leaks_push(TALLOC_CTX *ctx);
#define check_leaks_pop(ctx) _check_leaks_pop((ctx), __location__)
diff --git a/src/tests/leak_check.c b/src/tests/leak_check.c
index 79c8f05f6a3b9a1893d442af4aa67510f1e7a957..fa153eb543b0ed78559a30ac3ea6b1b78088fa9d 100644
--- a/src/tests/leak_check.c
+++ b/src/tests/leak_check.c
@@ -50,8 +50,8 @@ const char *check_leaks_err_msg(void)
return leak_err_msg;
}
-bool
-_check_leaks(TALLOC_CTX *ctx, size_t bytes, const char *location)
+static bool
+check_leaks(TALLOC_CTX *ctx, size_t bytes, const char *location)
{
size_t bytes_allocated;
@@ -110,7 +110,7 @@ _check_leaks_pop(TALLOC_CTX *ctx, const char *location)
}
talloc_zfree(snapshot);
- return _check_leaks(old_ctx, bytes_allocated, location);
+ return check_leaks(old_ctx, bytes_allocated, location);
}
bool
@@ -140,7 +140,7 @@ leak_check_teardown(void)
_set_leak_err_msg("Exiting with a non-empty stack");
return false;
}
- res = check_leaks(global_talloc_context, 0);
+ res = check_leaks(global_talloc_context, 0, __location__);
talloc_disable_null_tracking();
talloc_free(global_talloc_context);
return res;
--
2.5.0
-------------- next part --------------
>From d9e428b18c3282fc877683b1a8228665c5f9d48a Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Sat, 24 Oct 2015 15:48:26 +0200
Subject: [PATCH 5/5] TESTS: Add warning for unused result of leak check
functions
---
configure.ac | 17 +++++++++++++++++
src/tests/common.h | 16 +++++++++++++---
2 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/configure.ac b/configure.ac
index 51c0b0da8a7cfb692eabf38a71e42fbb0e40b4b0..c457879795c77dcb46f8d2bf6cc33c4bb4e36c27 100644
--- a/configure.ac
+++ b/configure.ac
@@ -379,6 +379,23 @@ if test x"$sss_cv_attribute_format" = xyes ; then
[whether compiler supports __attribute__((format))])
fi
+AC_CACHE_CHECK([whether compiler supports __attribute__((warn_unused_result))],
+ sss_cv_attribute_warn_unused_result,
+ [AC_COMPILE_IFELSE(
+ [AC_LANG_SOURCE(
+ [ char _check_leaks(int bytes) __attribute__ ((warn_unused_result)); ]
+ )],
+ [sss_cv_attribute_warn_unused_result=yes],
+ [
+ AC_MSG_RESULT([no])
+ AC_MSG_WARN([compiler does NOT support __attribute__((warn_unused_result))])
+ ])
+ ])
+if test x"$sss_cv_attribute_warn_unused_result" = xyes ; then
+ AC_DEFINE(HAVE_FUNCTION_ATTRIBUTE_WARN_UNUSED_RESULT, 1,
+ [whether compiler supports __attribute__((warn_unused_result))])
+fi
+
PKG_CHECK_MODULES([CHECK], [check >= 0.9.5], [have_check=1], [have_check=])
if test x$have_check = x; then
AC_MSG_WARN([Without the 'CHECK' libraries, you will be unable to run all tests in the 'make check' suite])
diff --git a/src/tests/common.h b/src/tests/common.h
index ac9ba40263fb9d253157c7d07c90dbba04484a74..794218c5b4646bd42d216d9bf45715e10c9ba866 100644
--- a/src/tests/common.h
+++ b/src/tests/common.h
@@ -25,11 +25,20 @@
#ifndef __TESTS_COMMON_H__
#define __TESTS_COMMON_H__
+#include "config.h"
+
#include <talloc.h>
#include "util/util.h"
#include "providers/data_provider.h"
#include "providers/ldap/sdap.h"
+
+#ifdef HAVE_FUNCTION_ATTRIBUTE_WARN_UNUSED_RESULT
+#define SSS_ATTRIBUTE_WARN_UNUSED_RESULT __attribute__((warn_unused_result))
+#else
+#define SSS_ATTRIBUTE_WARN_UNUSED_RESULT
+#endif
+
#define N_ELEMENTS(arr) (sizeof(arr) / sizeof(arr[0]))
extern TALLOC_CTX *global_talloc_context;
@@ -37,10 +46,11 @@ extern TALLOC_CTX *global_talloc_context;
void check_leaks_push(TALLOC_CTX *ctx);
#define check_leaks_pop(ctx) _check_leaks_pop((ctx), __location__)
-bool _check_leaks_pop(TALLOC_CTX *ctx, const char *location);
+bool _check_leaks_pop(TALLOC_CTX *ctx, const char *location)
+ SSS_ATTRIBUTE_WARN_UNUSED_RESULT;
-bool leak_check_setup(void);
-bool leak_check_teardown(void);
+bool leak_check_setup(void) SSS_ATTRIBUTE_WARN_UNUSED_RESULT;
+bool leak_check_teardown(void) SSS_ATTRIBUTE_WARN_UNUSED_RESULT;
const char *check_leaks_err_msg(void);
void tests_set_cwd(void);
--
2.5.0
More information about the sssd-devel
mailing list