bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: another hash cleanup


From: Jim Meyering
Subject: Re: another hash cleanup
Date: Fri, 19 Jun 2009 09:19:37 +0200

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 <address@hidden>
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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]