-2

I've got a list of unique-ptrs which, upon clearing the list, gives the following error:

Unhandled exception at 0x013EA350 in Last.exe: 0xC0000005: Access violation reading location 0xFEEEFEEE.

This is triggered in the std::list file, when iterating through the elements. The problem is a little unique:

  1. A MenuManager has a list of unique_ptr<Menu> objects.
  2. A Menu has a list of unique_ptr<MenuItem> objects.
  3. The first Menu and all of its MenuItems destruct perfectly, however this error is triggered during the clearing of the second Menu in the MenuManager's list, with completely independent MenuItems. It appears as if the second unique_ptr<Menu> has become undefined after the destruction of the first, although it was present and non-null at the beginning of the MenuManager's destruction.

What is causing this? (Not, specifically, where in my code I'm causing this error. I'm wondering what the general cause of this could be)

MenuManager.h

class MenuManager
{
public:
    /*MenuManager(list<Menu*> menus) 
        : m_menus(menus) { SetMenu( menus.front() ); }*/
    MenuManager() : m_currentMenu(nullptr) {}


    // Get the menu that the manager is pointing to.
    Menu* GetCurrentMenu(void) { return m_currentMenu; }

    // Set the current menu based on its name.
    void SetMenu(string menuName) { SetMenu( GetMenuByName(menuName) ); }
    // Add a menu to the list of menus managed by this object.
    void AddMenu(Menu* menu);

    // Render the current menu.
    void RenderMenu(void) { m_currentMenu->Render(); }

private:
    list< unique_ptr<Menu> > m_menus; // The list of menus managed by this object
    Menu* m_currentMenu; // A pointer to the menu in current use

    Menu* GetMenuByName(string menuName);
    void SetMenu(Menu* menu); // Set the menu based on the menu argument
};

Menu is a subclass of another, largely irrelevant class. However, its list is defined as list< unique_ptr<MenuItem> >. MenuItem's definition is inconsequential.

JordanBell
  • 140
  • 1
  • 2
  • 11
  • 1
    Please show the code. It sounds like there's a bug in the code you're describing. – Drew Dormann Jun 14 '14 at 23:16
  • 1
    Please post the definitions of `Menu` and `MenuItem` – Praetorian Jun 14 '14 at 23:20
  • Alright, will do. Though there's really not much more to it than that. – JordanBell Jun 14 '14 at 23:22
  • That doesn't look like the definitions of `Menu` and `MenuItem` to me. Is `m_currentMenu` dynamically allocated, and does `MenuManager` own it? – Praetorian Jun 14 '14 at 23:30
  • Sorry for kidding around. Try adding `assert(m_currentMenu)` in `RenderMenu`. If that doesn't turn up anything, try doing the same sort of investigation in other places. – Howard Hinnant Jun 14 '14 at 23:31
  • @Praetorian I'm not adding Menu and MenuItem because they are inconsequentially small pieces of code that subclass already known working classes that would only distract from the main problem. Yes and yes to the last two questions. – JordanBell Jun 14 '14 at 23:35
  • @Howard, I understand the basics of debugging. I can also accept "this is not something easily solved at a glance, I can't tell you what's wrong." as an answer. Seems that is the case. – JordanBell Jun 14 '14 at 23:36
  • @Tedium Judging from the code in `MenuManager`, your *inconsequentially small pieces of code* could also be violating the rule of three. `MenuManager` even lacks a destructor to `delete m_currentMenu;`! And once you add that, have you thought about what would happen if you copied a `MenuManager` instance. You're probably doing something similar in the other two classes, but since you're unwilling to post them, all I can do is wish you the best of luck. – Praetorian Jun 14 '14 at 23:39
  • 1
    Fair enough, +1 on the question to offset the impatience of others. Most of what you've posted is declaration, not definition. If you can narrow this problem down to something small enough to post (but is a stand-alone compilable program), that still exhibits the symptom, I'm sure we can diagnose it. – Howard Hinnant Jun 14 '14 at 23:40
  • @Howard Hinnant fair advice, I'll see if I can narrow something down more helpful. – JordanBell Jun 14 '14 at 23:46
  • @Praetorian You seem to understand much about the code you simultaneously criticize for not knowing enough about. The destructor is absent to differ to the default destruction of its members. And I'll admit, I'm wary to explicitly delete m_currentMenu, as I fear that, in my unfamiliarity, I would also delete the object it points at. Seeing as that object would be wrapped within a unique_ptr within the list, I could only imagine that would cause problems. If you think that that is an indicator of what I have done wrong, please show me the way. If it is just criticism for the sake of it, well... – JordanBell Jun 14 '14 at 23:50
  • Well, that's why I asked whether `MenuManager` owns `m_currentMenu`. From your last comment, it seems it doesn't. There's some other `unique_ptr` responsible for deleting it. If that's the case then `MenuManager` absolutely shouldn't be deleting it within its destructor, and there's nothing obviously wrong with your code. You can either try further debugging or posting the definitions of those functions. And I still maintain `Menu` and `MenuItem` may not be as inconsequential as you think they are. – Praetorian Jun 14 '14 at 23:57
  • 1
    0xFEEEFEEE means the memory was already freed. http://stackoverflow.com/questions/127386/in-visual-studio-c-what-are-the-memory-allocation-representations – drescherjm Jun 15 '14 at 00:44

1 Answers1

1

You do not show enough of the code to pinpoint the bug, but the error speaks for itself...

Unhandled exception at 0x013EA350 in Last.exe: 0xC0000005: Access violation reading location 0xFEEEFEEE.

In debug mode under Visual Studio, memory that gets freed is set to the code 0xFEEE. So the pointers are being freed twice.

Note that I see that the manager has an std::list of pointers that it is not actually managing: you have an AddMenu() with a bare pointer.

I strongly suggest that you replace your std::unique_ptr with std::shared_ptr, everywhere, and never use a bare pointer anywhere (Except to call lower level API which requires such bare pointers.) The huge advantage of a shared pointer is that you can have many classes holding a reference to it and if one class dies, the others still have a valid pointer. If you have parent/children references, make sure to use an std::weak_ptr for the parent pointer in the child.

Alexis Wilke
  • 19,179
  • 10
  • 84
  • 156
  • Blind use of "shared_ptr everywhere" without the design of an acyclic graph of memory ownership virtually assures memory leaks. I see that you advise `weak_ptr` for children that point back to their parents to break cyclic dependencies, and that is good. But insufficient. Cyclic memory ownership can be much more subtle than parent/child relationships. And the advice of "shared_ptr everywhere" just exacerbates the problem. A diagnosable runtime crash is a blessing compared to an undiagnosable memory leak. There is no substitute for careful understanding and design. – Howard Hinnant Jun 15 '14 at 02:18
  • shared_ptr may be part of the solution. But it should not be applied blindly. – Howard Hinnant Jun 15 '14 at 02:19
  • I think that in his case that's will resolve his problem, he may then end up with some memory leaks, but frankly all the projects where I introduced shared pointers, I now have no leaks either way. And at this point it doesn't look like he's handling exceptions, whereas shared pointers do that for you automatically. – Alexis Wilke Jun 15 '14 at 03:16
  • The question I asked was actually, "What's causing this?", and Alexis Wilke has answered this perfectly for me with "the pointers are being freed twice." Looks like it'd help a lot if I read up on the meanings of each of the Visual Studio codes, should help problems like this in the future. Thanks Alexis, that gives me somewhere to start debugging. Unfortunately, I already replaced the relevant unique_ptrs with shared_ptrs, only to have the exact same error, so the object must have been deleted some other way. Thanks again for the guidance. I should really start using smart pointers ;) – JordanBell Jun 15 '14 at 16:28