On (24/02/16 14:01), Lukas Slebodnik wrote: >On (24/02/16 12:47), Petr Cech wrote: >>On 02/23/2016 03:28 PM, Lukas Slebodnik wrote: >>>On (23/02/16 08:28), Petr Cech wrote: >>>>On 02/19/2016 04:33 PM, Lukas Slebodnik wrote: >>>> >>>>[...] >>>> >>>>>>>Hello Lukas, >>>>>>> >>>>>>>thank you for careful code review. I addressed all comments and I found >>>>>>>memory leak in original colondb tool. Patch of memory leak is attached in >>>>>>>patch #1 with brief but explaining description in commit message. >>>>>>> >>>>>>>Regards >>>>>>>-- >>>>>>>Petr^4 Cech >>>>>>From 2bce55008b3f1a070c224bbb25191408efa42fee Mon Sep 17 00:00:00 2001 >>>>>>>From: Petr Cech<pc...@redhat.com> >>>>>>>Date: Thu, 18 Feb 2016 06:33:53 -0500 >>>>>>>Subject: [PATCH 1/3] COLONDB: Fix memory leak >>>>>>> >>>>>>>man 3 getline says: >>>>>>>ssize_t getline(char **lineptr, size_t *n, FILE *stream); >>>>>>>If *lineptr is set to NULL and *n is set 0 before the call, then >>>>>>>getline() will allocate a buffer for storing the line. This buffer >>>>>>>should be freed by the user program even if getline() failed. >>>>>>> >>>>>>>This patch fix buffer freeing in case if getline() failed. >>>>>>> >>>>>Nice catch. >>>>> >>>>>ACK >>>>Thank you, Lukas. >>>> >>>>>>From 016afc2aff7b980ba56690b6bafbcab377cf1e80 Mon Sep 17 00:00:00 2001 >>>>>>>From: Petr Cech<pc...@redhat.com> >>>>>>>Date: Tue, 24 Nov 2015 10:34:10 -0500 >>>>>>>Subject: [PATCH 2/3] COLONDB: Add comments on functions >>>>>>> >>>>>>>The colondb API provides three function: >>>>>>>* sss_colondb_open() >>>>>>>* sss_colondb_write_field() >>>>>>>* sss_colondb_read_field() >>>>>>> >>>>>>>It is not obvious that sss_colondb_open() add destructor on talloc >>>>>>>context which close the colondb during free context. And there is >>>>>>>expectation that SSS_COLONDB_SENTINEL is type of last item in line. >>>>>>> >>>>>>>So this patch adds simple lightening comments in doxygen style. >>>>>>> >>>>>>>Resolves: >>>>>>>https://fedorahosted.org/sssd/ticket/2764 >>>>>>>--- >>>>>ACK >>>>Thanks. >>>> >>>>>>From 036ffc7e61456baa9bbb112452bbcc483e0e07c3 Mon Sep 17 00:00:00 2001 >>>>>>>From: Petr Cech<pc...@redhat.com> >>>>>>>Date: Fri, 27 Nov 2015 06:39:37 -0500 >>>>>>>Subject: [PATCH 3/3] TEST_TOOLS_COLONDB: Add tests for sss_colondb_* >>>>>>> >>>>>>>There are three functions at API of colondb wrapper: >>>>>>>* sss_colondb_open() >>>>>>>* sss_colondb_readline() >>>>>>>* sss_colondb_writeline() >>>>>>> >>>>>>>This patch adds tests for all of them. >>>>>>> >>>>>>>We test those cases: >>>>>>>* open nonexisting file for read >>>>>>>* open nonexisting file for write >>>>>>>* open existing empty file for read >>>>>>>* open existing file with records for read >>>>>>>* open existing empty file for write >>>>>>>* open existing file with records for write >>>>>>>* write to empty file >>>>>>>* write to file with existing records >>>>>>>* sss_colondb_open() >>>>>>>* sss_colondb_readline() >>>>>>>* sss_colondb_write_line() >>>>>>>* write to empty file and read it >>>>>>> >>>>>>>Resolves: >>>>>>>https://fedorahosted.org/sssd/ticket/2764 >>>>>I'm not sure that talloc leak checking is properly used in this unit test. >>>> >>>>I have rewrote leak checking. I hope I have removed all memory leaks from my >>>>tests. >>>> >>>>>There are leaks in setup functions: >>>>>diff --git a/src/tests/cmocka/test_tools_colondb.c >>>>>b/src/tests/cmocka/test_tools_colondb.c >>>>>index 11274d9..9cbb17f 100644 >>>>>--- a/src/tests/cmocka/test_tools_colondb.c >>>>>+++ b/src/tests/cmocka/test_tools_colondb.c >>>>>@@ -61,7 +61,7 @@ static void create_empty_file(void **state, const char >>>>>*path, const char *name) >>>>> assert_non_null(fp); >>>>> fclose(fp); >>>>> >>>>>- //talloc_free(tmp_ctx); >>>>>+ talloc_free(tmp_ctx); >>>>> } >>>>> >>>>> static void create_nonempty_file(void **state, >>>>>@@ -86,7 +86,7 @@ static void create_nonempty_file(void **state, >>>>> >>>>> sss_colondb_writeline(db, table); >>>>> >>>>>- //talloc_free(tmp_ctx); >>>>>+ talloc_free(tmp_ctx); >>>>> } >>>>> >>>>> >>>>>BTW we do not use c++ style comments:-) >>>>>It's already in old coding style. >>>>>https://www.freeipa.org/page/Coding_Style#Comments >>>> >>>>Of course, it was my inattention. Sorry. >>>> >>>>>and moreover there is a leak in sss_colondb_writeline. >>>>>@see attached patch. >>>>> >>>>>I didn't try to find out what's wrong with leak check. >>>>>If you you are not able to fix it yourself then >>>>>we might take a look at test together to save some time >>>>>sending/reviewing patches. >>>>> >>>>>LS >>>>> >>>>> >>>>>0001-TOOLS-Fix-minor-memory-leak-in-sss_colondb_writeline.patch >>>>> >>>>> >>>>> From fd458598cb619ec24ef0d00a47d84c790631257b Mon Sep 17 00:00:00 2001 >>>>>From: Lukas Slebodnik<lsleb...@redhat.com> >>>>>Date: Fri, 19 Feb 2016 16:18:02 +0100 >>>>>Subject: [PATCH 1/2] TOOLS: Fix minor memory leak in sss_colondb_writeline >>>>> >>>>>The variable line was initialized to NULL. >>>>>The we created temporary context tmp_ctx. >>>>>We use talloc_asprintf_append to append string to line which is initially >>>>>NULL and therefore new context which was not connected to tmp_ctx. >>>>> man 3 talloc_string -> talloc_asprintf_append >>>>>--- >>>>> src/tools/common/sss_colondb.c | 7 +++++++ >>>>> 1 file changed, 7 insertions(+) >>>>> >>>>>diff --git a/src/tools/common/sss_colondb.c >>>>>b/src/tools/common/sss_colondb.c >>>>>index >>>>>a1a52c552b949076bdedae58f275d29a176be1e6..e8aeb315c9ed0efde15553e2d741d04c5d895b1a >>>>> 100644 >>>>>--- a/src/tools/common/sss_colondb.c >>>>>+++ b/src/tools/common/sss_colondb.c >>>>>@@ -202,6 +202,13 @@ errno_t sss_colondb_writeline(struct sss_colondb *db, >>>>> return ENOMEM; >>>>> } >>>>> >>>>>+ line = talloc_strdup(tmp_ctx, ""); >>>>>+ if (line == NULL) { >>>>>+ DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed.\n"); >>>>>+ ret = ENOMEM; >>>>>+ goto done; >>>>>+ } >>>>>+ >>>>> for (i = 0; table[i].type != SSS_COLONDB_SENTINEL; i++) { >>>>> switch (table[i].type) { >>>>> case SSS_COLONDB_UINT32: >>>>>-- 2.5.0 >>>> >>>>Nice catch, ACK >>>> >>>> >>>>During code review there was problem with build on debian, so I did CI >>>>tests: >>>>http://sssd-ci.duckdns.org/logs/job/37/59/summary.html >>>> >>>> >>>>There are 4 patches attached. The first of them is from Lukas. This order of >>>>patches is valid. >>>> >>>>Regards >>>>-- >>>>Petr^4 Cech >>> >>>>From 1e85c2d3dec7958356ddfb3b4b8b221816e4848e Mon Sep 17 00:00:00 2001 >>>>From: Petr Cech <pc...@redhat.com> >>>>Date: Fri, 27 Nov 2015 06:39:37 -0500 >>>>Subject: [PATCH 4/4] TEST_TOOLS_COLONDB: Add tests for sss_colondb_* >>>> >>>>There are three functions at API of colondb wrapper: >>>>* sss_colondb_open() >>>>* sss_colondb_readline() >>>>* sss_colondb_writeline() >>>> >>>>This patch adds tests for all of them. >>>> >>>>We test those cases: >>>>* open nonexisting file for read >>>>* open nonexisting file for write >>>>* open existing empty file for read >>>>* open existing file with records for read >>>>* open existing empty file for write >>>>* open existing file with records for write >>>>* write to empty file >>>>* write to file with existing records >>>>* sss_colondb_open() >>>>* sss_colondb_readline() >>>>* sss_colondb_write_line() >>>>* write to empty file and read it >>>> >>>>Resolves: >>>>https://fedorahosted.org/sssd/ticket/2764 >>>>--- >>>>Makefile.am | 17 ++ >>>>src/tests/cmocka/test_tools_colondb.c | 471 >>>>++++++++++++++++++++++++++++++++++ >>>>2 files changed, 488 insertions(+) >>>>create mode 100644 src/tests/cmocka/test_tools_colondb.c >>>> >>>>diff --git a/Makefile.am b/Makefile.am >>>>index >>>>8ff3322cd4aaaf8d419b664a18c4e0e67205beb8..4e4f38a5eaf5bfa2cfafa88b9b32848e6c27131b >>>> 100644 >>>>--- a/Makefile.am >>>>+++ b/Makefile.am >>>>@@ -241,6 +241,7 @@ if HAVE_CMOCKA >>>> pam-srv-tests \ >>>> test_ipa_subdom_util \ >>>> test_ipa_subdom_server \ >>>>+ test_tools_colondb \ >>>> test_krb5_wait_queue \ >>>> test_cert_utils \ >>>> test_ldap_id_cleanup \ >>>>@@ -2576,6 +2577,22 @@ test_ipa_subdom_server_LDADD = \ >>>> libdlopen_test_providers.la \ >>>> $(NULL) >>>> >>>>+test_tools_colondb_SOURCES = \ >>>>+ src/tests/cmocka/test_tools_colondb.c \ >>>>+ src/tools/common/sss_colondb.c \ >>>>+ $(NULL) >>>>+test_tools_colondb_CFLAGS = \ >>>>+ $(AM_CFLAGS) \ >>>>+ $(NULL) >>>>+test_tools_colondb_LDFLAGS = \ >>>>+ $(NULL) >>>>+test_tools_colondb_LDADD = \ >>>>+ $(CMOCKA_LIBS) \ >>>>+ $(SSSD_INTERNAL_LTLIBS) \ >>>>+ $(POPT_LIBS) \ >>>>+ libsss_test_common.la \ >>>>+ $(NULL) >>>>+ >>>>test_krb5_wait_queue_SOURCES = \ >>>> src/tests/cmocka/common_mock_be.c \ >>>> src/tests/cmocka/test_krb5_wait_queue.c \ >>>>diff --git a/src/tests/cmocka/test_tools_colondb.c >>>>b/src/tests/cmocka/test_tools_colondb.c >>>>new file mode 100644 >>>>index >>>>0000000000000000000000000000000000000000..123ad39bd75a0206fbf152fd35a10c6113f36bfc >>>>--- /dev/null >>>>+++ b/src/tests/cmocka/test_tools_colondb.c >>>>@@ -0,0 +1,471 @@ >>>>+/* >>>>+ Authors: >>>>+ Petr Čech <pc...@redhat.com> >>>>+ >>>>+ Copyright (C) 2015 Red Hat >>>>+ >>>>+ This program is free software; you can redistribute it and/or modify >>>>+ it under the terms of the GNU General Public License as published by >>>>+ the Free Software Foundation; either version 3 of the License, or >>>>+ (at your option) any later version. >>>>+ >>>>+ This program is distributed in the hope that it will be useful, >>>>+ but WITHOUT ANY WARRANTY; without even the implied warranty of >>>>+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>>>+ GNU General Public License for more details. >>>>+ >>>>+ You should have received a copy of the GNU General Public License >>>>+ along with this program. If not, see <http://www.gnu.org/licenses/>. >>>>+*/ >>>>+ >>>>+#include <talloc.h> >>>>+#include <errno.h> >>>>+#include <popt.h> >>>>+ >>>>+#include "tests/cmocka/common_mock.h" >>>>+#include "src/tools/common/sss_colondb.h" >>>>+ >>>>+#define TESTS_PATH "tp_" BASE_FILE_STEM >>>>+#define TESTS_FILE "test_colondb.ldb" >>>>+ >>>>+const char *TEST_STRING1 = "white"; >>>>+const int TEST_INT1 = 12; >>>>+ >>>>+const char *TEST_STRING2 = "black"; >>>>+const int TEST_INT2 = 34; >>>>+ >>>>+static void create_dir(const char *path) >>>>+{ >>>>+ errno_t ret; >>>>+ >>>>+ errno = 0; >>>>+ ret = mkdir(path, 0775); >>>>+ assert_int_equal(ret, 0); >>>It's better to use assert_return_code for system calls. >>>it will also print errno in cese of failure. >>Well. >> >>>>+} >>>>+ >>>>+static void create_empty_file(void **state, const char *path, const char >>>>*name) >>>>+{ >>>>+ TALLOC_CTX *tmp_ctx = NULL; >>>>+ char *file_name = NULL; >>>>+ FILE *fp = NULL; >>>>+ >>>>+ tmp_ctx = talloc_new(*state); >>> ^^^^^^ >>> state was not initialized yet. >>> and this function was called with >>> "create_empty_file(state ...)" >>> >>> The stare is initialized at the end of "setup" >>> function. Basically it means that you allocate on >>> NULL >>> context. So it would be better to use already >>> created >>> test_ctx in setup function which is protected with >>> leak_check. >>Good idea. I didn't recognize it. >> >>> >>>>+ assert_non_null(tmp_ctx); >>>>+ >>>>+ create_dir(path); >>>>+ >>>>+ file_name = talloc_asprintf(tmp_ctx, "%s/%s", path, name); >>>>+ assert_non_null(file_name); >>>>+ >>>>+ fp = fopen(file_name, "w"); >>>>+ assert_non_null(fp); >>>>+ fclose(fp); >>>>+ >>>>+ talloc_free(tmp_ctx); >>>>+} >>>>+ >>>>+static void create_nonempty_file(void **state, >>>>+ const char *path, const char *name) >>>>+{ >>>>+ TALLOC_CTX *tmp_ctx = NULL; >>>>+ struct sss_colondb *db = NULL; >>>>+ struct sss_colondb_write_field table[] = { >>>>+ { SSS_COLONDB_STRING, { .str = TEST_STRING2 } }, >>>>+ { SSS_COLONDB_UINT32, { .uint32 = TEST_INT2 } }, >>>>+ { SSS_COLONDB_SENTINEL, { 0 } } >>>>+ }; >>>>+ >>>>+ tmp_ctx = talloc_new(*state); >>>>+ assert_non_null(tmp_ctx); >>>>+ >>>>+ create_empty_file(state, TESTS_PATH, TESTS_FILE); >>>>+ >>>>+ db = sss_colondb_open(tmp_ctx, SSS_COLONDB_WRITE, >>>>+ TESTS_PATH "/" TESTS_FILE); >>>>+ assert_non_null(db); >>>>+ >>>>+ sss_colondb_writeline(db, table); >>> missing assert for writeline. >>Nice catch. >> >>> >>>>+ talloc_free(db); >>>>+ talloc_free(tmp_ctx); >>>>+} >>>>+ >>>>+static int setup(void **state, int file_state) >>>>+{ >>>>+ TALLOC_CTX *test_ctx = NULL; >>>>+ >>>>+ assert_true(leak_check_setup()); >>>>+ >>>>+ check_leaks_push(global_talloc_context); >>>>+ test_ctx = talloc_new(global_talloc_context); >>>>+ assert_non_null(test_ctx); >>>>+ >>>>+ switch (file_state) { >>>>+ case 0: >>>>+ break; >>>>+ case 1: >>>>+ create_empty_file(state, TESTS_PATH, TESTS_FILE); >>>>+ break; >>>>+ case 2: >>>>+ create_nonempty_file(state, TESTS_PATH, TESTS_FILE); >>>As I mentioned earlier, It would be good to paste "test_ctx" >>>instead of state which is initialized before return. >>> >>>>+ break; >>>>+ default: >>>>+ break; >>>>+ } >>>>+ >>>>+ *state = test_ctx; >>>>+ >>>BTW there is lot of code duplication in tests. >>>There is check_leaks_push in the beginning >>>and at the end of test. >>> >>>If we move it to the end of setup function and to the >>>beginning of teardown functions then test will be simpler. >>> >>>the 1st check_leaks_push in setup will check for leaks in setup >>>function and 2nd check_leaks_push will check for leaks in tests. >>> >>>>+ return 0; >>>>+} >>>>+ >>>>+static int without_file_setup(void **state) >>>>+{ >>>>+ return setup(state, 0); >>>>+} >>>>+ >>>>+static int with_empty_file_setup(void **state) >>>>+{ >>>>+ return setup(state, 1); >>>>+} >>>>+ >>>>+static int with_nonempty_file_setup(void **state) >>>>+{ >>>>+ return setup(state, 2); >>>>+} >>>>+ >>>>+static int teardown(void **state) >>>>+{ >>>>+ errno_t ret; >>>>+ >>>>+ errno = 0; >>>>+ ret = unlink(TESTS_PATH "/" TESTS_FILE); >>>>+ if (ret != 0) { >>>>+ assert_int_equal(errno, ENOENT); >>>>+ } >>>>+ >>>>+ talloc_zfree(*state); >>>>+ test_dom_suite_cleanup(TESTS_PATH, NULL, NULL); >>>>+ assert_true(check_leaks_pop(global_talloc_context)); >>>>+ assert_true(leak_check_teardown()); >>>>+ >>>>+ return 0; >>>>+} >>>>+ >>>//snip >>> >>>>+void test_read_from_nonempty(void **state) >>>>+{ >>>>+ TALLOC_CTX *test_ctx = NULL; >>>>+ TALLOC_CTX *test_ctx_child = NULL; >>>>+ struct sss_colondb *db = NULL; >>>>+ errno_t ret; >>>>+ const char *string; >>>>+ uint32_t number; >>>>+ struct sss_colondb_read_field table[] = { >>>>+ { SSS_COLONDB_STRING, { .str = &string } }, >>>>+ { SSS_COLONDB_UINT32, { .uint32 = &number } }, >>>>+ { SSS_COLONDB_SENTINEL, { 0 } } >>>>+ }; >>>>+ >>>>+ test_ctx = talloc_new(*state); >>>>+ check_leaks_push(test_ctx); >>>>+ >>>>+ db = sss_colondb_open(test_ctx, SSS_COLONDB_READ, >>>>+ TESTS_PATH "/" TESTS_FILE); >>>>+ assert_non_null(db); >>>>+ >>>>+ test_ctx_child = talloc_new(test_ctx); >>>test_ctx_child is not required here. You just need to >>>release "string" after readline. >>>"test_ctx_child" could hide leaks in sss_colondb_readline. >>Thanks, that was pain for me. test_ctx_child was hack. >> >>>>+ ret = sss_colondb_readline(test_ctx_child, db, table); >>>>+ assert_int_equal(ret, 0); >>>>+ assert_string_equal(string, TEST_STRING2); >>>>+ assert_int_equal(number, TEST_INT2); >>>>+ >>>>+ talloc_free(test_ctx_child); >>>>+ >>>>+ talloc_free(db); >>>>+ assert_true(check_leaks_pop(test_ctx)); >>>>+} >>> >>>Attached diff should address all mentioned issues. >>> >>>LS >> >>Lukas, thanks for diff. I learned many thinks. New patches are attached. >> >ACK > >http://sssd-ci.duckdns.org/logs/job/37/88/summary.html > master: * b590f44c06158485357d69cc5b24d5af05f1bb95 * cf1109e30320a994187edeb438ac7cdc36f0dd2b * 2dd75ea79a57615808754c0ce550786edbc17d69 * 6977d7c84145ac69195be58b3330861b9b8a3b72
sssd-1-13: * b269edafff139510ee1e9c00bdbc8f27e8aea691 * fbf7d5683287fa2c7b450b8f5b0df63673f25d83 * 34ba0c53d0d966c64ea11a6269cdd0ad985f4068 * d75ac50d0c065974a7ec2330f60657ae85e487c0 LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
