Why does my program return garbage when accessing the value of a pointer?

Question

When the below code is run, I get garbage output. I have debugged it enough to figure out the error comes when I try to access hobbies[i]->hobby. Any help would be appreciated. I have been trying to figure out what is going on for hours.

int Graph::addUserToHobby(std::string hobby, std::string id){
  int key = ((int)hobby[0] + (int)hobby[1])%HASHMAP_SIZE;
  int collisions = 0;
  while(hobbies[key] != NULL && hobbies[key]->hobby.compare(hobby) != 0 ){
    key++;
    collisions++;
    if(key >= HASHMAP_SIZE){
      key = 0;
    }
  }

  if(hobbies[key] == NULL){
    hobbylist hob;
    hob.hobby = hobby;
    hob.list.push_back(findVertex(id));
    hobbies[key] = &hob;
  }
  else{
    hobbies[key]->list.push_back(findVertex(id));
  }
  return collisions;

}

void Graph::displayHobbies(){

  for(int i=0; i<HASHMAP_SIZE; i++){
    if(hobbies[i] != NULL){
      cout << hobbies[i]->hobby << ": ";
      for(unsigned int j=0; j<hobbies[i]->list.size()-1; j++){
        cout << hobbies[i]->list[j]->name << ", ";
      }
      cout <<  hobbies[i]->list[hobbies[i]->list.size()-1]->name << endl;
    }
  }
}

hobbies[] is an array of pointers. I want to put hob in that array as a pointer. — Charles Candon, Dec 11 '18 at 08:08
Assuming you have `Hobby *hobbies[HASHMAP_SIZE];` somewhere, replace it with `std::map hobbies;` and everything will become much simpler. — Ken Y-N, Dec 11 '18 at 08:15
Why `hobbies` contain pointers and not the `hobbylist` objects themselves? If you insist on pointers (which implies additional memory and runtime overhead), consider using smart ones, such as `std::unique_ptr`. — Daniel Langr, Dec 11 '18 at 08:18
@DanielLangr Wouldn't `std::shared_ptr` be a better choice here? — Ken Y-N, Dec 11 '18 at 08:21
you are not accessing the value of the pointer, but you are dereferencing the pointer to access the value of the pointee. — 463035818_is_not_a_number, Dec 11 '18 at 08:22
@KenY-N Why? I don't see any reason for `std::shared_ptr`, which has much higher memory/runtime overhead with reference counting and type-erased deleters. But we don't see all the program code. — Daniel Langr, Dec 11 '18 at 08:24
To the close-voter: I am pretty sure the problem was not a typo. — gsamaras, Dec 11 '18 at 09:11

gsamaras · Answer 1 · 2018-12-11T11:50:03.820

Focus your attention in that part of the code:

if(hobbies[key] == NULL) {
  hobbylist hob;
  ...        
  hobbies[key] = &hob;
}

When hob gets out of scope (at the end of that if-statement's body), hobbies[key] will reference something that doesn't exist any more.

Later on in your program, as you correctly noticed, when you do cout << hobbies[i]->hobby;, you will request for hobby on something that has gone out of scope, which invokes Undefined Behavior (UB).

Some possible solutions:

Use an std::map, instead of the array of pointers you use now. The container will automatically take care of the memory management for you. (Recommended)
Use smart pointers (e.g. std::unique_ptr), instead of raw pointers. Read more in What is a smart pointer and when should I use one?
Dynamically allocate hob, so that its lifetime is extended (that means that when that if-statement's body terminates, hob's lifetime won't terminate). This approach requires you to be responsible for the memory management (you have to de-allocate every piece of memory that you dynamically allocated before (call delete as many times as you called new)).

This is why vectors or other containers are preferred to than `C` style arrays! — Francis Cugler, Dec 11 '18 at 08:12

score 2 · Answer 2 · answered Dec 11 '18 at 08:08

In this part:

if(hobbies[key] == NULL){
  hobbylist hob;
  /* ... */
  hobbies[key] = &hob;
}

hob is allocated on the stack and deleted after the if block. So the pointer you have in hobbies[key] is dangling. You can catch these sort of errors with valgrind.

donkopotamus · Answer 3 · 2018-12-11T08:18:59.697

2

Your issue is that you are populating your hobbies value with pointers to objects allocated on the stack.

These objects will have subsequently been destroyed. Perhaps you were meaning to allocate them on the heap with new?

hobbylist* hob = new hobbylist;
...
hobbies[key] = hob

edited Dec 11 '18 at 08:18

answered Dec 11 '18 at 08:09

donkopotamus

18,536
2
36
52

Why does my program return garbage when accessing the value of a pointer?

3 Answers3