Fetching value from std::map which is a user defined class object leading to invalid read

Question

For Simplicity I am creating a demo program which simulates the problem.

In below program when I try access map using worksFineWithPointer function everything works fine i.e. by store the address of value fetched from map and results are also expected and when I try to access map using problem function i.e. store the value in object, it does not shows expected results.

I tried running my faulty function with Valgrind it showed lot of invalid read request. But understanding valgrind is quite difficult for me so didn't got much (if an easy book can be sugested or online reference that would be great too). If I comment out the problem function no valgrind errors.

Then I compiled the program with electric fence and it immediately core dumped and I got to know that which line is having a issue which is as below:

#10 0x0000000000401721 in Node::operator= (this=0x7ffccb72e720, obj=...) at test.cc:33
#11 0x0000000000401b83 in A::problem (this=0x7ffccb72e7c8, prefix="test") at test.cc:102
#12 0x0000000000401350 in main () at test.cc:114
(gdb) f 11
#11 0x0000000000401b83 in A::problem (this=0x7ffccb72e7c8, prefix="test") at test.cc:102
102                 crawler = crawler.getMap()[ch];

Not sure what is wrong in above line it will just call operator= and values should get copied in crawler object. May be I am missing some concept of C++ here.

Below is the full code

#include <iostream>
#include <map>

using namespace std;

class Node
{
    private:
        map<char, Node> mymap;
        bool test;

    public:
        Node():test(false){}

        bool getTest()
        {
            return test;
        }

        void setTest()
        {
            test = true;
        }

        map<char, Node>& getMap()
        {
            return mymap;
        }
};

class A {
    private:
        Node* root;

    public:
    A()
    {
        root = new Node();
    }

    void insert(string word)
    {
        Node* crawler = root;
        for (char ch : word)
        {
            if (crawler->getMap().find(ch) == crawler->getMap().end())
                crawler->getMap()[ch] = Node();

            crawler = &crawler->getMap()[ch];
        }
        crawler->setTest();
    }

    // This way if I access no valgring errors everything works fine
    void worksFineWithPointer(string prefix)
    {
        Node* crawler = root;
        cout << "worksFineWithPointer:" << endl;

        for (char ch : prefix)
        {
            crawler = &crawler->getMap()[ch];
            cout << crawler->getTest() << endl;
        }

    }

    // Problematic Function but not able to find why it is wrong
    void problem(string prefix)
    {
        Node crawler = *root;
        cout << "Boolean Attribute changed to true when using objects instead of pointer:" << endl;

        for (char ch : prefix)
        {
            crawler = crawler.getMap()[ch];
            cout << crawler.getTest() << endl;
        }
    }
};

int main()
{
    A a;
    a.insert("test");
    a.problem("test");
    a.worksFineWithPointer("test");

    return 0;
}

Output (When Compiled and ran with Efence no output came as it dumped before printing):

Boolean Attribute changed to true when using objects instead of pointer:
1 <<<< how this changed to true
1 <<<< how this changed to true
1 <<<< how this changed to true
1 <<<< this was anyway true

WorksFineWithPointer:
0
0
0
1

I am missing some basic C++ concept here I guess, it would be great if someone could explain what is the issue with problematic code statement.

Answer 1

In your problem function, you are making copies of the Nodes and then iterating through them. That's not the same thing as iterating through the pointers.

But the real issue I think is with your class design

class Node
{
    private:
        map<char, Node> mymap;
// ...
};

At this point, Node is an incomplete type, and so you can't have a map of that type. (From c++17, I believe a vector of incomplete types is allowed though). For a map, I think it's ill-formed, so even if your code compiles, it has UB, and why it crashes is irrelevant.

Anyway, I think you'd be better off having a map of char to Node*. Then your class looks like this

class Node
{
    private:
        map<char, Node*> mymap;
        bool test;
    public:
// ...   
      map<char, Node*>& getMap()
        {
            return mymap;
        }
};

And then your functions become

  void worksFineWithPointer(string prefix)
    {
        Node* crawler = root;
        cout << "worksFineWithPointer:" << endl;

        for (char ch : prefix)
        {
            crawler = crawler->getMap()[ch];
            cout << crawler->getTest() << endl;
        }

    }

and

    void problem(string prefix) // no longer
    {
        Node crawler = *root;
        cout << "Works fine with objects too :)" << endl;

        for (char ch : prefix)
        {
            crawler = *crawler.getMap()[ch];
            cout << crawler.getTest() << endl;
        }
    }

and they should both work fine.

Even better, make mymap a map<char, std::unique_ptr<Node>>.