di-set: Fix memory leak

di-set: Fix memory leak

[Bruno Haible]

Building a gnulib testdir with CC="gcc -fsanitize=leak", I see this finding
in 'test-di-set':

ERROR: LeakSanitizer: detected memory leaks

Direct leak of 80 byte(s) in 1 object(s) allocated from:
    #0 0x7f874eea8858 in __interceptor_malloc ../../../../gcc-8.2.0/libsanitizer/lsan/lsan_interceptors.cc:52
    #1 0x401aee in hash_initialize ../../gllib/hash.c:605

Direct leak of 16 byte(s) in 1 object(s) allocated from:
    #0 0x7f874eea8858 in __interceptor_malloc ../../../../gcc-8.2.0/libsanitizer/lsan/lsan_interceptors.cc:52
    #1 0x40259e in ino_map_insert ../../gllib/ino-map.c:133

Indirect leak of 20432 byte(s) in 1 object(s) allocated from:
    #0 0x7f874eea9404 in __interceptor_calloc ../../../../gcc-8.2.0/libsanitizer/lsan/lsan_interceptors.cc:74
    #1 0x401b59 in hash_initialize ../../gllib/hash.c:626

Indirect leak of 32 byte(s) in 2 object(s) allocated from:
    #0 0x7f874eea8858 in __interceptor_malloc ../../../../gcc-8.2.0/libsanitizer/lsan/lsan_interceptors.cc:52
    #1 0x40259e in ino_map_insert ../../gllib/ino-map.c:133

SUMMARY: LeakSanitizer: 20560 byte(s) leaked in 5 allocation(s).
FAIL test-di-set (exit status: 23)


It is confirmed by valgrind:

$ valgrind --tool=memcheck --num-callers=20 --leak-check=yes --leak-resolution=high --show-reachable=yes ./test-di-set 
==5309== Memcheck, a memory error detector
==5309== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==5309== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==5309== Command: ./test-di-set
==5309== 
==5309== 
==5309== HEAP SUMMARY:
==5309==     in use at exit: 20,560 bytes in 5 blocks
==5309==   total heap usage: 27 allocs, 22 frees, 314,688 bytes allocated
==5309== 
==5309== 16 bytes in 1 blocks are indirectly lost in loss record 1 of 5
==5309==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5309==    by 0x40202E: ino_map_insert (ino-map.c:133)
==5309==    by 0x400BDF: di_set_insert (di-set.c:231)
==5309==    by 0x40073A: main (test-di-set.c:39)
==5309== 
==5309== 16 bytes in 1 blocks are indirectly lost in loss record 2 of 5
==5309==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5309==    by 0x40202E: ino_map_insert (ino-map.c:133)
==5309==    by 0x400BDF: di_set_insert (di-set.c:231)
==5309==    by 0x400757: main (test-di-set.c:40)
==5309== 
==5309== 16 bytes in 1 blocks are definitely lost in loss record 3 of 5
==5309==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5309==    by 0x40202E: ino_map_insert (ino-map.c:133)
==5309==    by 0x400BDF: di_set_insert (di-set.c:231)
==5309==    by 0x40079F: main (test-di-set.c:47)
==5309== 
==5309== 20,432 bytes in 1 blocks are indirectly lost in loss record 4 of 5
==5309==    at 0x4C2FB55: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5309==    by 0x401563: hash_initialize (hash.c:626)
==5309==    by 0x401F96: ino_map_alloc (ino-map.c:90)
==5309==    by 0x400AEB: map_inode_number.isra.1 (di-set.c:208)
==5309==    by 0x400BDF: di_set_insert (di-set.c:231)
==5309==    by 0x40073A: main (test-di-set.c:39)
==5309== 
==5309== 20,544 (80 direct, 20,464 indirect) bytes in 1 blocks are definitely lost in loss record 5 of 5
==5309==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5309==    by 0x4014EE: hash_initialize (hash.c:605)
==5309==    by 0x401F96: ino_map_alloc (ino-map.c:90)
==5309==    by 0x400AEB: map_inode_number.isra.1 (di-set.c:208)
==5309==    by 0x400BDF: di_set_insert (di-set.c:231)
==5309==    by 0x40073A: main (test-di-set.c:39)
==5309== 
==5309== LEAK SUMMARY:
==5309==    definitely lost: 96 bytes in 2 blocks
==5309==    indirectly lost: 20,464 bytes in 3 blocks
==5309==      possibly lost: 0 bytes in 0 blocks
==5309==    still reachable: 0 bytes in 0 blocks
==5309==         suppressed: 0 bytes in 0 blocks
==5309== 
==5309== For counts of detected and suppressed errors, rerun with: -v
==5309== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)


This patch fixes it.


2019-03-10  Bruno Haible  <bruno@clisp.org>

	di-set: Fix memory leak.
	* lib/di-set.c (di_set_free): Free the ino_map through ino_map_free(),
	not free().

diff --git a/lib/di-set.c b/lib/di-set.c
index e8f69db..2c0601e 100644
--- a/lib/di-set.c
+++ b/lib/di-set.c
@@ -136,7 +136,7 @@ void
 di_set_free (struct di_set *dis)
 {
   hash_free (dis->dev_map);
-  free (dis->ino_map);
+  ino_map_free (dis->ino_map);
   free (dis->probe);
   free (dis);
 }

Re: di-set: Fix memory leak

[Bernhard Voelker]

[-- Attachment #1: Type: text/plain, Size: 1519 bytes --]

On 3/10/19 2:20 PM, Bruno Haible wrote:
> 2019-03-10  Bruno Haible  <bruno@clisp.org>
> 
> 	di-set: Fix memory leak.
> 	* lib/di-set.c (di_set_free): Free the ino_map through ino_map_free(),
> 	not free().
> 
> diff --git a/lib/di-set.c b/lib/di-set.c
> index e8f69db..2c0601e 100644
> --- a/lib/di-set.c
> +++ b/lib/di-set.c
> @@ -136,7 +136,7 @@ void
>  di_set_free (struct di_set *dis)
>  {
>    hash_free (dis->dev_map);
> -  free (dis->ino_map);
> +  ino_map_free (dis->ino_map);
>    free (dis->probe);
>    free (dis);
>  }

This commit (3703dbbe88dd) makes coreutils' du(1) crash.

Reproducer:
---------------------------------------------------------
#!/bin/sh
set -x
git clone --depth=1 git://git.sv.gnu.org/gnulib.git \
  && cd gnulib \
  && rm -rf /tmp/testdir/ \
  && ./gnulib-tool --create-testdir --with-tests --dir=/tmp/testdir di-set \
  && cd /tmp/testdir \
  && ./configure --quiet \
  && make \
  && patch -p0 --fuzz=0 <<<'\
--- gltests/test-di-set.c.ORIG	2019-04-10 08:23:09.663882510 +0200
+++ gltests/test-di-set.c	2019-04-18 22:27:57.778264454 +0200
@@ -27,6 +27,9 @@
 {
   struct di_set *dis = di_set_alloc ();
   ASSERT (dis);
+  di_set_free (dis);
+  dis = di_set_alloc ();
+  ASSERT (dis);

   ASSERT (di_set_lookup (dis, 2, 5) == 0); /* initial lookup fails */
   ASSERT (di_set_insert (dis, 2, 5) == 1); /* first insertion succeeds */
' \
&& make check
---------------------------------------------------------

The attached allows for 'ino-map' to free on NULL.

Have a nice day,
Berny



[-- Attachment #2: 0001-ino-map-allow-free-on-NULL.patch --]
[-- Type: text/x-patch, Size: 1303 bytes --]

From c6f24d39f25cbedfba6f728da48e9666d7240894 Mon Sep 17 00:00:00 2001
From: Bernhard Voelker <mail@bernhard-voelker.de>
Date: Thu, 18 Apr 2019 23:04:26 +0200
Subject: [PATCH] ino-map: allow free on NULL

* lib/ino-map.c (ino_map_free): Do nothing when MAP is NULL.
Since commit 3703dbbe88dd, di_set_free calls ino_map_free which may
be NULL.
---
 ChangeLog     | 7 +++++++
 lib/ino-map.c | 9 ++++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 398c33968..1d96402d8 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2019-04-18  Bernhard Voelker  <mail@bernhard-voelker.de>
+
+	ino-map: allow free on NULL.
+	* lib/ino-map.c (ino_map_free): Do nothing when MAP is NULL.
+	Since commit 3703dbbe88dd, di_set_free calls ino_map_free which may
+	be NULL.
+
 2019-04-14  Paul Eggert  <eggert@cs.ucla.edu>
 
 	* lib/str-two-way.h: Fix comment typo.
diff --git a/lib/ino-map.c b/lib/ino-map.c
index c48defa65..9d6bd0894 100644
--- a/lib/ino-map.c
+++ b/lib/ino-map.c
@@ -105,9 +105,12 @@ ino_map_alloc (size_t next_mapped_ino)
 void
 ino_map_free (struct ino_map *map)
 {
-  hash_free (map->map);
-  free (map->probe);
-  free (map);
+  if (map)
+    {
+      hash_free (map->map);
+      free (map->probe);
+      free (map);
+    }
 }
 
 
-- 
2.21.0

Re: di-set: Fix memory leak

[Bruno Haible]

Hi Berny,

I would prefer to see the fix in lib/di-set.c, not lib/ino-map.c.
Rationale: in 99% of the uses of the 'ino-map' module, the NULL
check is not needed.

The change to tests/test-di-set.c looks good. If you can commit
that as well?

Thanks.

Apologies for the bug.

Bruno

Re: di-set: Fix memory leak

[Bernhard Voelker]

[-- Attachment #1: Type: text/plain, Size: 482 bytes --]

Hi Bruno,

On 4/18/19 11:42 PM, Bruno Haible wrote:
> I would prefer to see the fix in lib/di-set.c, not lib/ino-map.c.
> Rationale: in 99% of the uses of the 'ino-map' module, the NULL
> check is not needed.

Fine by me. ;-)

BTW: the _GL_ATTRIBUTE_NONNULL decl of ino_map_free would have
made problems anyway ...

> The change to tests/test-di-set.c looks good. If you can commit
> that as well?

Sure, thanks - new patch attached - du(1) works again. ;-)

Have a nice day,
Berny

[-- Attachment #2: 0001-di-set-allow-free-with-ino_map-being-NULL.patch --]
[-- Type: text/x-patch, Size: 1923 bytes --]

From 22b911f63ca11395f03ce2ce3cc0b371a37a576d Mon Sep 17 00:00:00 2001
From: Bernhard Voelker <mail@bernhard-voelker.de>
Date: Thu, 18 Apr 2019 23:04:26 +0200
Subject: [PATCH] di-set: allow free with 'ino_map' being NULL

* lib/di-set.c (di_set_free): Avoid ino_map_free() when dis->ino_map
is NULL.  Bug introduced in commit 3703dbbe88dd.
* tests/test-di-set.c: Add di_set_free() right after di_set_alloc()
as a test.
---
 ChangeLog           | 8 ++++++++
 lib/di-set.c        | 3 ++-
 tests/test-di-set.c | 3 +++
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 398c33968..916d154eb 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2019-04-18  Bernhard Voelker  <mail@bernhard-voelker.de>
+
+	di-set: allow free with 'ino_map' being NULL.
+	* lib/di-set.c (di_set_free): Avoid ino_map_free() when dis->ino_map
+	is NULL.  Bug introduced in commit 3703dbbe88dd.
+	* tests/test-di-set.c: Add di_set_free() right after di_set_alloc()
+	as a test.
+
 2019-04-14  Paul Eggert  <eggert@cs.ucla.edu>
 
 	* lib/str-two-way.h: Fix comment typo.
diff --git a/lib/di-set.c b/lib/di-set.c
index 2c0601e03..47ade5821 100644
--- a/lib/di-set.c
+++ b/lib/di-set.c
@@ -136,7 +136,8 @@ void
 di_set_free (struct di_set *dis)
 {
   hash_free (dis->dev_map);
-  ino_map_free (dis->ino_map);
+  if (dis->ino_map)
+    ino_map_free (dis->ino_map);
   free (dis->probe);
   free (dis);
 }
diff --git a/tests/test-di-set.c b/tests/test-di-set.c
index 6887be3b6..0f492c837 100644
--- a/tests/test-di-set.c
+++ b/tests/test-di-set.c
@@ -27,6 +27,9 @@ main (void)
 {
   struct di_set *dis = di_set_alloc ();
   ASSERT (dis);
+  di_set_free (dis); /* free with dis->ino_map still being NULL */
+  dis = di_set_alloc ();
+  ASSERT (dis);
 
   ASSERT (di_set_lookup (dis, 2, 5) == 0); /* initial lookup fails */
   ASSERT (di_set_insert (dis, 2, 5) == 1); /* first insertion succeeds */
-- 
2.21.0