Segfault after deleting elements from vector

Question

I'm currently working on a project, more precisely a tangram game. I have a problem with a segfault and I don't understand why.

Given that I have a whole project, I will try to simplify the problem : I have a GameManager class that contains in particular a Menu object (and other things, but I don't think that is important. The gameManager is used to inititialize this object and manage it. The Menu contains a vector of Button(each button has a lambda to perform an action when the user click on it).

std::vector<std::unique_ptr<Button>> buttons;

To illustrate how it works I will take an example : if the user clicks on the "Load" button, the gameManager delete the current Buttons contained in the menu and add new buttons in that menu.

void GameManager::initMainMenuButtons() {
...
menu -> addButton(std::unique_ptr<Button>(new Button(x1, y1, x2, y2, "Create",
    [this]{
        std::cout << "Create level" << std::endl;
        menu->clear()
        initCreateLevelButtons();
        actionManager->setMenu(menu);
    }
)));
...
}

In that code sample, I have a method initMainMenuButtons that adds several buttons in the menu , like "Load" or "Quit". When the user clicks on "Create", I want to change the interface (adding and deleting buttons). So, to delete the buttons, i call the method clear()

void Menu::clear() {
  buttons.clear();
  decorationPieces.clear(); // not interesting
}

I'm using unique_ptr, thus, I don't have to delete the buttons mannualy. So far, no problem : the vector of buttons seems to be empty (size is 0). Next, the method initCreateLevelButtons() is called. This method is very similar to initMainMenu : it adds buttons in the menu, nothing else. During this call, the buttons seems to be correctly added in the vector, I printed the content of the vector at the end and the vector contains the correct buttons.

And there, the problem appears : after the call of initCreateLevelButtons(), there is a segfault when i want to use the menu, so, actionManager->setMenu(menu); doesn't work. I tried to print the menu std::cout << menu << std::endl, and test if this pointer is nullptr, but it doesn't work either. I don't understand why the menu seems to be correct at the last line of initCreateLevelButtons() and becomes invalid just after. If I doesn't clear the vector of buttons (the menu->clear instruction), the program works, but, the last buttons are still here).

I tried to use raw pointers and I notices that the program is able to clear the vector as long as the buttons are not deleted (If I add a loop to delete the buttons, the problem arises), so, I conclued that the probleme is the buttons deleting. I don't understanf why, I'm stuck. I don't know if I explained it weel, because, as I have already said, the code is part of a whole project, it's hard to introduce classes without introduce other things. if you want details or the complete code of methods, I can provide them.

Welcome to SO, @Eradan. Please try to shorten your question and provide [minimal example](https://stackoverflow.com/help/minimal-reproducible-example) that reproduces your issue. It will not only help you to debug the issue, but also help us to help you. — Tarek Dakhran, Feb 17 '20 at 16:49
`menu->clear()` would cause `*this` (the "Create" button) to be destroyed, and `this` to become a dangling pointer. The two calls following it apparently access member variables through `this`. Your program therefore exhibits undefined behavior, by way of access an object whose lifetime has ended. — Igor Tandetnik, Feb 17 '20 at 16:51
Side note: removing items from a container while iterating the container requires care. Remove an item wrong and you break the iterators (and cannot safely proceed) or change the count (Take care not to overshoot the new end of the container). Look up [the Erase-Remove Idiom](https://en.wikipedia.org/wiki/Erase%E2%80%93remove_idiom). — user4581301, Feb 17 '20 at 16:53
Thanks for your answers, It's a bit complicated to give you a minimal example without give you the whole project^^. I will try to reproduce the problem on a easier way. — Eradan, Feb 17 '20 at 16:54
fwiw my personal rule of thumb is to never pass a lambda that captures `this` or via reference "upwards", or at least be extra cautious when doing so — 463035818_is_not_an_ai, Feb 17 '20 at 16:57

score 5 · Accepted Answer · answered Feb 17 '20 at 16:58

5

menu sustains lifetime of some button
button sustain lifetime of lambda
when you click button lambda clears menu
menu destructor clears button, button clears lambda
lambda continues execution when it in fact has been already destroyed -> undefined behavior ends with a crash

Now question is: Do you own Button class?
If yes then the easiest way to fix it, is to invoke copy of lambda in the button.

answered Feb 17 '20 at 16:58

Marek R

32,568
6
55
140

It works! Thanks! I was looking for the reason of the problem too far. I understood the problem and I fixed it, it was very simple actually, I blame myself for having blocked on this problem ^^, thanks again. – Eradan Feb 17 '20 at 17:05

score 3 · Answer 2 · answered Feb 17 '20 at 17:07

When you call menu->clear() it calls buttons.clear().

When you call buttons.clear() it destroys all the elements of buttons.

When you destroy the unique_ptr for the "Create" button, it destroys the "Create" button.

I assume button's callback is a std::function. When the button is destroyed, so is the std::function.

When the std::function is destroyed, your callback lambda object ([this]{...}) is destroyed.

The this pointer inside a lambda is stored in the lambda object. So now the memory that held the this pointer has been deallocated.

Since actionManager is a member variable of GameManager, actionManager->setMenu(menu) is really this->actionManager->setMenu(menu) which crashes because it uses a dangling pointer.

One workaround is to put the button code in a function of GameManager (since the GameManager is not destroyed), and call that from the lambda. Then, it's okay if you destroy the button while inside that function. It's okay to destroy an object whose code is currently running, as long as you are careful to not access the object after it's destroyed! This is also okay with std::function. I.e.:

    [this]{
        // move the rest of the code to the CreateLevel function
        this->CreateLevel();

        // At this point the lambda has been destroyed, but it's not a problem
        // because we don't do anything.
    }

Thanks for the solution :) I did not think about the fact that the lambda is destroyed, however it was obvious. I thought the problem was more complicated — Eradan, Feb 17 '20 at 17:15

Segfault after deleting elements from vector

2 Answers2