Eric Blake wrote:
> Jim Meyering <jim <at> meyering.net> writes:
>
>> > Aargh. Ten minutes after I push, I finally see my memory leak.
>> >
>> > Obviously, new_table needs to be freed before returning true.
>>
>> Hah! I should have looked at more than the patch.
>>
>> Is that code path already exercised by test-hash.c?
>> If so, I would have seen it eventually, via valgrind.
>
> Yes, for example, look at op 6 in the random hash ops section (although can
> you
> truly trust rand() to be predictable across systems?). A valgrind run would
> indeed catch it (but that's too mechanical - it's more fun to find memory
> leaks
> by code inspection ;)
>
> Here's what I'll push:
...
> diff --git a/lib/hash.c b/lib/hash.c
> index f2123b4..88ae32c 100644
> --- a/lib/hash.c
> +++ b/lib/hash.c
> @@ -863,7 +863,10 @@ hash_rehash (Hash_table *table, size_t candidate)
> if (new_table == NULL)
> return false;
> if (new_table->n_buckets == table->n_buckets)
> - return true;
> + {
> + free (new_table);
> + return true;
> + }
>
> /* Merely reuse the extra old space into the new table. */
> #if USE_OBSTACK
I see you did not push that (good!), presumably because you noticed it
too would leak, due to the malloc'd table->bucket. This is what you pushed:
if (new_table->n_buckets == table->n_buckets)
{
free (new_table->bucket);
free (new_table);
return true;
}
However, I have a slight preference for this, since it leaves us
with less duplication of implementation-specific details:
if (new_table->n_buckets == table->n_buckets)
{
hash_free (new_table);
return true;
}
It's on a cold code path, so the small amount of extra work isn't an issue.
Also, by using hash_free, we avoid a leak in the USE_OBSTACK code.
>From 4940f5a2d5d88262f2f243db4a7fa1bda70ad3b5 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyer...@redhat.com>
Date: Fri, 19 Jun 2009 09:17:49 +0200
Subject: [PATCH] hash: use hash_free directly, rather than open-coding part of
it
* lib/hash.c (hash_rehash): Use hash_free directly, to avoid a leak
in USE_OBSTACK code and for maintainability.
---
lib/hash.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/lib/hash.c b/lib/hash.c
index a81eec7..914cc05 100644
--- a/lib/hash.c
+++ b/lib/hash.c
@@ -858,8 +858,7 @@ hash_rehash (Hash_table *table, size_t candidate)
return false;
if (new_table->n_buckets == table->n_buckets)
{
- free (new_table->bucket);
- free (new_table);
+ hash_free (new_table);
return true;
}
--
1.6.3.2.406.gd6a466
