[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