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<pcech(a)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<pcech(a)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<pcech(a)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<lslebodn(a)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 <pcech(a)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 <pcech(a)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.