Dangling pointer (I think) causing program crash?

Question

Following my book (C++ Primer 5th edition), we are making a program with two classes: Message and Folder. The idea is that a Message will be stored in a Folder. The same Message can be saved in multiple Folders. In order to accomplish this, Message has a set of pointers to Folder that point to each of the Folder that contains it. Folder has a set of pointers to Message that point to each of the Messages it contains. Here is my code:

Message.hpp:

    #ifndef MESSAGE_HPP
#define MESSAGE_HPP
#include <string>
#include <set>
#include "Folder.hpp"

using std::string;
using std::set;

class Message
{
    
friend class Folder;
public:
    explicit Message(const string &str) : contents(str) {}
    Message(const Message&);
    Message& operator=(const Message&);
    ~Message();
    
    void save(Folder&);
    void remove(Folder&);
    
private:
    string contents;
    set<Folder*> folders;
    
    void addToFolders(const Message&);
    void removeFromFolders();
    void swap(Message&, Message&);
};

#endif // MESSAGE_HPP

Message.cpp:

#include "Message.hpp"

Message::Message(const Message &msg) : contents(msg.contents), folders(msg.folders) {
    addToFolders(msg);
}

Message::~Message() {
    removeFromFolders();
}

Message& Message::operator=(const Message &rhs) {
    removeFromFolders();
    contents = rhs.contents;
    folders = rhs.folders;
    addToFolders(rhs);
    return *this;
}

void Message::save(Folder &f) {
    folders.insert(&f);
    f.addMsg(this);
}

void Message::remove(Folder &f) {
    folders.erase(&f);
    f.remMsg(this);
}

void Message::addToFolders(const Message &msg) {
    for (auto f : msg.folders)
        f->addMsg(this);
}

void Message::removeFromFolders() {
    for (auto f : folders)
        f->remMsg(this);
}

void Message::swap(Message &lhs, Message &rhs) {
    using std::swap;
    for (auto f : lhs.folders)
        f->remMsg(&lhs);
    
    for (auto f: rhs.folders)
        f->remMsg(&rhs);
    
    swap(lhs.folders, rhs.folders);
    swap(lhs.contents, rhs.contents);
    
    for (auto f : lhs.folders)
        f->addMsg(&lhs);
        
    for (auto f : rhs.folders)
        f->addMsg(&rhs);
}

Folder.hpp:

#ifndef FOLDER_HPP
#define FOLDER_HPP
#include <set>
#include <string>

using std::string;
using std::set;

class Message;
class Folder
{

public:
    Folder(const string &str = "") : name(str) {}
    void addMsg(Message*);
    void remMsg(Message*);
    
private:
    string name;
    set<Message*> containedMessages;
};

#endif // FOLDER_HPP

Folder.cpp:

#include "Folder.hpp"

void Folder::addMsg(Message *msg) {
    containedMessages.insert(msg);
}

void Folder::remMsg(Message *msg) {
    containedMessages.erase(msg);
}

I test my program with this code in my main.cpp file:

int main() {
    Message msg("testing");
    Folder inbox;
    msg.save(inbox);
}

This program crashes. Playing with the program, looking at my debugger, and using a memory checker program, it appears the problem is ultimately coming from my remMsg function in the Folder.cpp file. But I can't figure out exactly what's happening. My memory checker gives errors that say unaddressable memory and invalid heap argument, so I know it has something to do with memory getting deleted and dangling pointers, but I can't seem to fix the program.

I've suspected that when reaching the end of the main function, my Folder object may be getting deleted before my Message object, which leaves the set of pointers that point to my folder dangling (this set is a data member of my Message object). I would need to have access to the Message class members in my Folder destructor to account this possibility, but I cannot #include my Message header in my Folder header, because the Folder header is already #included in my Message header.

Any tips?

You can't `erase()` from a `std::set` while iterating over it in this way, it invalidates the iterator. See https://stackoverflow.com/questions/6438086/iterator-invalidation-rules/6438087#6438087 and https://en.cppreference.com/w/cpp/container#Iterator_invalidation — Nate Eldredge, Feb 13 '21 at 00:36
This actually wasn't the problem. My Folder object was being deleted before my Message object resulting in dangling pointer. I fixed the problem. But after seeing your comment, I remember that I should not change the size of a container if I am iterating over it with a range-for loop. Why do my range-for loops not produce errors in this case? I would expect it to. — Jonathan Gafar, Feb 13 '21 at 01:05
More generally, having an instance of a class store a pointer or reference to a variable outside the class is a recipe for undefined behaviour. Particularly in this case, where the pointer points at a variable of automatic storage duration - so there is nothing to prevent the pointer outliving the lifespan of its pointee. Also, for variables of automatic storage duration, the standard requires that the order of destruction is the reverse of the order of construction when they pass out of scope - and storing a pointer to one object in another object does not affect that. — Peter, Feb 13 '21 at 06:46

score 1 · Answer 1 · answered Feb 13 '21 at 01:05

You are right. The main scope is trying to free the Folder created there and then the set that is receiving it as a pointer to it. It is taking ownership of it. You could, in main, Folder* inbox = new Folder(); and then msg.save(*inbox); (The ugliest approach) or use reference instead of pointer or std::shared_ptr() or such.

Please don't use the new, it is only going around the problem, and creating more problems for later, Quack!. Not a good practice.

If you are keen give it a go to std::shared_ptr in places where you would be using a raw pointer.

score 0 · Answer 2 · answered Feb 13 '21 at 01:03

My suspicion was correct. I used print statements to show that the Folder object was being deleted before the Message object, resulting in a dangling pointer. I merged my two classes into one header so that the Folder class has access to the Message class and I put this in the Folder destructor:

Folder::~Folder() {
    for (auto m : containedMessages)
        m->folders.erase(this);
}

Dangling pointer (I think) causing program crash?

2 Answers2