c++unordered-mapsegment

Segmentation fault when inserting elements in unordered_map using the [] operator


As the title says, when the unordered_map uses the [] operator to insert elements, there will be a Segmentation fault, and the most confusing place for me is that this error occurs when I use resize() on vec, and when I use push_back() The program has no problem . I don't understand what is causing this.

#include<iostream>
#include<unordered_map>
#include<vector>
#include<cstdlib>
#include<ctime>

using namespace std;

struct Element
{
  int key;
  vector<int> vec;
  int flag;
};

class Test
{
private:
  unordered_map<int,Element *> map;
public:
  Element *getElement(int key,int flag)
  {
    Element *element;
    auto temp = map.find(key);
    if(temp==map.end()||flag == temp->second->flag)
    {
      element = new Element();
      element->key = key;
      element->flag = flag;
      if(temp != map.end())
      {
        delete temp->second;
        map.erase(key);
      }
      map[key] = element;
    }
    else
    {
      element = map[key];
    }
    return element;
  }
};

int main()
{
  srand((unsigned)time(nullptr));
  Test test;
  for (size_t i = 0; i < 100000; i++)
  {
    /* code */
    int vecSize = rand()%100;
    int key = rand()%5000;
    int flag = rand()%5000;
    Element *element = test.getElement(key,flag);
    if(element->vec.size()==0)
    {
      element->vec.resize(vecSize,0);
    }
    for (size_t i = 0; i < vecSize; i++)
    {
      element->vec[i] = rand()%10000;
    }
  }
  return 0;
}

output:

Program received signal SIGSEGV, Segmentation fault.
0x000055555555716e in std::__detail::_Hash_node<std::pair<int const, Element*>, false>::_M_next (this=0x8470000046a) at /usr/include/c++/7/bits/hashtable_policy.h:298
298       { return static_cast<_Hash_node*>(this->_M_nxt); }
(gdb) bt
#0  0x000055555555716e in std::__detail::_Hash_node<std::pair<int const, Element*>, false>::_M_next (this=0x8470000046a) at /usr/include/c++/7/bits/hashtable_policy.h:298
#1  0x000055555555782c in std::_Hashtable<int, std::pair<int const, Element*>, std::allocator<std::pair<int const, Element*> >, std::__detail::_Select1st, std::equal_to<int>, std::hash<int>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<false, false, true> >::_M_rehash_aux (this=0x7fffffffe370, __n=337) at /usr/include/c++/7/bits/hashtable.h:2098
#2  0x0000555555556ef2 in std::_Hashtable<int, std::pair<int const, Element*>, std::allocator<std::pair<int const, Element*> >, std::__detail::_Select1st, std::equal_to<int>, std::hash<int>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<false, false, true> >::_M_rehash
    (this=0x7fffffffe370, __n=337, __state=@0x7fffffffe220: 167) at /usr/include/c++/7/bits/hashtable.h:2071
#3  0x00005555555564e0 in std::_Hashtable<int, std::pair<int const, Element*>, std::allocator<std::pair<int const, Element*> >, std::__detail::_Select1st, std::equal_to<int>, std::hash<int>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<false, false, true> >::_M_insert_unique_node (this=0x7fffffffe370, __bkt=88, __code=1424, __node=0x55555577a330) at /usr/include/c++/7/bits/hashtable.h:1718
#4  0x0000555555555992 in std::__detail::_Map_base<int, std::pair<int const, Element*>, std::allocator<std::pair<int const, Element*> >, std::__detail::_Select1st, std::equal_to<int>, std::hash<int>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<false, false, true>, true>::operator[] (this=0x7fffffffe370, __k=@0x7fffffffe2f4: 1424) at /usr/include/c++/7/bits/hashtable_policy.h:728
#5  0x00005555555554db in std::unordered_map<int, Element*, std::hash<int>, std::equal_to<int>, std::allocator<std::pair<int const, Element*> > >::operator[] (this=0x7fffffffe370, 
    __k=@0x7fffffffe2f4: 1424) at /usr/include/c++/7/bits/unordered_map.h:973
#6  0x00005555555551ca in Test::getElement (this=0x7fffffffe370, key=1424, flag=318) at main.cpp:35
#7  0x0000555555554e23 in main () at main.cpp:56

Solution

  • Let's take a look at your major design decision:

      unordered_map<int,Element *> map;
    

    You are using a "bare pointer", which should be reserved for cases where the data structure, meaning your map, needs to refer to something, but it doesn't own it. This is not the case in your program: no one else owns the Element objects. So you should store them explicitly, and return references:

    unordered_map<int,Element> map;
    
    Element& getElement(int key,int flag);
    

    As other people have also indicated: your program can be both simpler, and correct. You're making life too hard for yourself by not writing proper C++.

    EDIT because you indicate that you're stuck with a legacy API: It would still be possible to preserve the API but redo the implementation. If the API requires you to return a * pointer to internals (rather than a reference as I suggested), store the elements internally as smart pointers, and return the get() result from that.