help-gplusplus
[Top][All Lists]
Advanced

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

Re: hash_set core dump on memory free


From: James Kanze
Subject: Re: hash_set core dump on memory free
Date: 5 Nov 2006 10:03:08 -0500

{ Note: this article is cross-posted to comp.lang.c++, 
microsoft.public.vc.stl, gnu.g++.help and comp.lang.c++.moderated. -mod }

Rakesh wrote:

>   What is wrong this implementation? I get a core dump at the free()
> statement? Thanks

> #include <ext/hash_map>
> #include <iostream.h>
> #include <ext/hash_set>

> using namespace std;
> using namespace __gnu_cxx;

> struct eqstr
> {
>   bool operator()(char* s1, char* s2) const
>   {
>     return strcmp(s1, s2) == 0;
>   }
> };

> int main()
> {
>
>   char *s, *s1, *temp;
>   hash_set<char *, hash<char *>, eqstr> AddedPhrases;
>   hash_set<char*, hash<char*>, eqstr> ::iterator Iter1;

>   s = (char *)malloc(sizeof(char)*100);
>   strcpy(s, "apple");
>   AddedPhrases.insert(s);

>   s1 = (char *)malloc(sizeof(char)*100);
>   strcpy(s1, "absent");
>   AddedPhrases.insert(s1);
>   for (Iter1 = AddedPhrases.begin(); Iter1 != AddedPhrases.end();
> Iter1++)
>   {
>          temp = *Iter1;
>          //printf("\nDeleting:%s:%d", temp, strlen(temp));
>          free(temp);

I'd be very surprised that this doesn't result in undefined
behavior.  You're leaving an invalide pointer in the table.
You're in for some very unplaisant surprises the next time some
other value hashes to this value, and the implementation tries
invoking your compare function on the entry.  (More generally,
changing anything which affects the value of anything used for
actually keying causes undefined behavior.)

In fact, a quick analysis of the g++ code in the library shows
that it does re-calculate the hash code in the ++ operator.
(The actual implementation seems to be more space optimized than
performance optimized---although space optimizing the iterator
does mean that it copies a lot faster.)  The result is that
anything can happen.  You might actually iterator through the
loop only once, or you might iterator more times than there are
elements in the loop, or you might loop endlessly over the same
element, or core dump in the iterator, or ...

The correct way to clean up a container like this would be
something like:

     __gnu_cxx::hash_set::iterator i = AddedPhrases.begin() ;
     while ( i != AddedPhrases.end() ) {
         char*               tmp = *i ;
         i = AddedPhrases.erase( i ) ;
         free( tmp ) ;
     }

>   }
> }

More generally, of course, you'd be better off:

  -- Using the standard IO (<iostream>, <ostream>); g++ definitly
     supports it, and the only justification today to use
     <iostream.h> is to support legacy compilers (g++ pre-3.0,
     Sun CC 4.2, etc.---all so old you shouldn't be using them
     anyway).

  -- Using std::string, instead of the char* junk.  Do that, and
     the destructor of the hash_set will clean up everything
     automatically.

  -- Not declaring variables until you can correctly initialize
     them.

--
James Kanze (Gabi Software)            email: james.kanze@gmail.com
Conseils en informatique orientée objet/
                    Beratung in objektorientierter Datenverarbeitung
9 place Sémard, 78210 St.-Cyr-l'École, France, +33 (0)1 30 23 00 34


-- 
      [ See http://www.gotw.ca/resources/clcm.htm for info about ]
      [ comp.lang.c++.moderated.    First time posters: Do this! ]



reply via email to

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