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