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

Question

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

The major, contributing, factor to your crash is a problem that's called "pointless use of pointers". There is no apparent reason for the map to store pointers. `unordered_map map;` will work just as well. And together with a default constructor for `Element` that initializes all of its members to their default values, this will cut the size of the code at least in half, and reduce greatly the breeding grounds for many different kinds of bugs. "Just Say No" to pointless use of pointers! — Sam Varshavchik, Apr 24 '22 at 14:12
One major problem is the loop where you set `element->vec[i]`. What if the `element` pointer you got was created earlier? Then its vector size could be different from `vecSize`. — Some programmer dude, Apr 24 '22 at 14:15
In fact, by using `try_emplace`, and an appropriate `Element` constructor, `getElement()` becomes just ***one*** line of code! This is the damage that pointless use of pointers creates, what a shame... — Sam Varshavchik, Apr 24 '22 at 14:16
@OP Even without `try_emplace`, `getElement()` could be 3 or 4 lines by not using pointers. So even with the optimal method @SamVarshavchik mentions, even a suboptimal rewrite reduces that function down by a large amount *if you didn't use pointers*. Also, I was going to ask you "what is this code supposed to do?". Whatever it is, on the very surface of what you attempted to do, you must have had an inkling that "this must have been done already, and much simpler". So this is a research issue as well. — PaulMcKenzie, Apr 24 '22 at 14:18
Another problem I see is a possible out of bounds access of the vector's. The outer `for` loop is calculating a new `vecSize` on each iteration, but the inner `for` loop uses `vecSize` to access vector elements without guaranteeing that preexisting vectors are actually that size, only empty vectors are resized. The statement `if(element->vec.size()==0)` should be `if(element->vec.size() < vecSize)` instead. — Remy Lebeau, Apr 24 '22 at 20:04

Victor Eijkhout · Accepted Answer · 2022-04-25T17:28:56.700

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.

Sorry, this is the code for an old project I'm maintaining and I also crash every now and then, thanks for your answer. — clove682, Apr 24 '22 at 14:27
It would still be possible to preserve the API but rejig the implementation. If the API requires you to return a `*` pointer to internals (rather than a reference as I suggested), store them as smart pointers, and return the `get()` result from that. — Victor Eijkhout, Apr 24 '22 at 14:45

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

1 Answers1