C++ crash when pushing self-removing back to a vector

Question

I want to genetically recreate a given image
I try to do this in sfml.
I have created a self-parting square that tries to evolve to look
like source image
Sadly, this thing crashes and I have no idea why, I suppose everything is handled nice and the vector appending shouldn't be a problem.
Please check out the code:
The main function:

#include "divisablesquare.h"
#include <SFML/Graphics.hpp>
#include <iostream>
#include <cstring>
#include <string>
#include <error.h>
#include <algorithm>

namespace GLOBAL
{
    bool DEBUG_MODE = false;
};

int IDX = 0;

int main(int argc, char * argv[])
{

    srand(time(NULL));
    std::string def;
    for(int i = 1; i < argc; i++)
    {
        def = argv[i];
        std::string def2 = def;
        std::transform(def2.begin(), def2.end(), def2.begin(), ::tolower);

        if(strcmp(def2.c_str(), "--debug") == 0)
        {
            GLOBAL::DEBUG_MODE = true;
            std::cerr << "Running in debug mode" << std::endl;
        }
        else
        {
            break;
        }
    }
    sf::Image sourceImage;
    sf::Texture sample;
    if(!sourceImage.loadFromFile(def) && GLOBAL::DEBUG_MODE)
    {
       std::cerr << "Failed to open specified image!" << std::endl;
    }
    sample.loadFromImage(sourceImage);
    sf::RectangleShape sourceRect;
    sourceRect.setSize((sf::Vector2f)sourceImage.getSize());
    sourceRect.setTexture(&sample);
    sf::RenderWindow mainWindow(sf::VideoMode(sourceImage.getSize().x*2+10, sourceImage.getSize().y), "Genetic Image Generator");
    std::vector<DivisableSquare> dSquares;
    {
        DivisableSquare starter(&dSquares, &sourceImage);
        starter.init(128, 128, 128, sourceImage.getSize().x, sourceImage.getSize().y, sourceImage.getSize().x + 10, 0);
        starter.Shape.setPosition({(float)sourceImage.getSize().x + 10, 0});
        starter.Shape.setFillColor({128,128,128});
        starter.Shape.setSize({(float)sourceImage.getSize().x, (float)sourceImage.getSize().y});
        dSquares.push_back(starter);
    }
    sf::Clock clock;

    while(mainWindow.isOpen())
    {
        sf::Time elapsed = clock.getElapsedTime();

        if(elapsed.asMilliseconds() > 1000)
        {

            clock.restart();
            dSquares.at(rand() % dSquares.size()).Partup();

        }
        sf::Event mainEvent;
        while(mainWindow.pollEvent(mainEvent))
        {
            if(mainEvent.type == sf::Event::Closed)
                mainWindow.close();
        }
        mainWindow.clear();
        mainWindow.draw(sourceRect);
        for(auto &&ref: dSquares)
        {
            mainWindow.draw(ref.Shape);
        }
        mainWindow.display();
    }
}

divisablesquare header:

#ifndef DIVISABLESQUARE_H
#define DIVISABLESQUARE_H
#include <vector>
#include <SFML/Graphics.hpp>


class DivisableSquare
{
private:
    sf::Image * parentImage;
    std::vector<DivisableSquare> * ParentContainter;
    unsigned short red, green, blue;
    double width, height;
    double posX, posY;
    int id;

public:
    ~DivisableSquare();
    sf::RectangleShape Shape;
    DivisableSquare(std::vector<DivisableSquare>*, sf::Image*);
    void init(unsigned short, unsigned short, unsigned short, unsigned int, unsigned int, unsigned int, unsigned int);
    void Partup();
};

#endif // DIVISABLESQUARE_H

and the c++ file:

#include "divisablesquare.h"
#include <random>
#include <algorithm>
#include <iostream>

extern int IDX;

DivisableSquare::DivisableSquare(std::vector<DivisableSquare> *pc, sf::Image*tp)
{
    this->ParentContainter = pc;
    this->parentImage = tp;
    this->id = IDX;
    IDX++;
}


DivisableSquare::~DivisableSquare()

{

}


void DivisableSquare::init(unsigned short r, unsigned short g, unsigned short b,
                           unsigned int width, unsigned int height, unsigned int posX, unsigned int posY)
{
    this->red = r;
    this->blue = b;
    this->green = g;
    this->width = width;
    this->height = height;
    this->posX = posX;
    this->posY = posY;
}

void DivisableSquare::Partup()
{

    if(this->width < 2 && this->height < 2)
        return;

    double percentCut = (rand()%60 + 20)/100;
    bool horizontalCut = rand()%2;

    double posX1, posX2;
    double posY1, posY2;
    double width1, width2;
    double height1, height2;



    if(horizontalCut)
    {
        posX1 = this->posX;
        posX2 = (this->posX+this->width)*percentCut;
        posY1 = this->posY;
        posY2 = this->posY;
        width1 = this->width*percentCut;
        width2 = this->width*(1-percentCut);
        height1 = this->height;
        height2 = this->height;
    }
    else
    {
        posX1 = this->posX;
        posX2 = this->posX;
        posY1 = this->posY;
        posY2 = (this->posY + this->height)*percentCut;
        width1 = this->width;
        width2 = this->width;
        height1 = this->height*percentCut;
        height2 = this->height*(1-percentCut);
    }

    struct RGB
    {
        float r, g, b;
        float parentCmp;
        float originalCmp;
        float averageCmp;
        /**
          * Make sure to append originalCmp later
          * also remove "= 0"
          * DONE
          */

    };

    std::vector<RGB> originalPixels1;
    std::vector<RGB> originalPixels2;

    for(unsigned int i = posX1; i < posX1+width1; i++)
    {
        for(unsigned int j = posY1; j < posY1+height1; j++)
        {
            if(this->parentImage->getSize().x > i && this->parentImage->getSize().y > j)
            {
                RGB pixel;
                pixel.r = this->parentImage->getPixel(i, j).r;
                pixel.g = this->parentImage->getPixel(i, j).g;
                pixel.b = this->parentImage->getPixel(i, j).b;
                originalPixels1.push_back(pixel);
            }
        }
    }



    for(unsigned int i = posX2; i < posX2+width2; i++)
    {
        for(unsigned int j = posY2; j < posY2+height2; j++)
        {
            if(this->parentImage->getSize().x > i && this->parentImage->getSize().y > j)
            {
                RGB pixel;
                pixel.r = this->parentImage->getPixel(i, j).r;
                pixel.g = this->parentImage->getPixel(i, j).g;
                pixel.b = this->parentImage->getPixel(i, j).b;
                originalPixels2.push_back(pixel);
            }
        }
    }

    RGB pix1 = {0,0,0,0,0,0}, pix2={0,0,0,0,0,0};

    for(auto &&ref : originalPixels1)
    {
        pix1.r += ref.r;
        pix1.g += ref.g;
        pix1.b += ref.b;
    }



    pix1.r /= originalPixels1.size();
    pix1.g /= originalPixels1.size();
    pix1.b /= originalPixels1.size();


    for(auto &&ref : originalPixels2)
    {
        pix2.r += ref.r;
        pix2.g += ref.g;
        pix2.b += ref.b;
    }


    pix2.r /= originalPixels1.size();
    pix2.g /= originalPixels1.size();
    pix2.b /= originalPixels1.size();

    auto comparVal = [](RGB v1, RGB v2)
    {
        float val1 = 0.2126*v1.r + 0.7152*v1.g + 0.0722*v1.b;
        float val2 = 0.2126*v2.r + 0.7152*v2.g + 0.0722*v2.b;
        return (val1 > val2) ? val1-val2 : val2-val1;
    };//smaller - better

    RGB first[100];
    RGB second[100];


    for(int i = 0; i < 100; i++)
    {
        first[i].r = rand() % 255;
        first[i].g = rand() % 255;
        first[i].b = rand() % 255;
        second[i].r = rand() % 255;
        second[i].g = rand() % 255;
        second[i].b = rand() % 255;
    }

    //  insert orginalcmp here

    for(int i = 0; i < 100; i++)
    {
        first[i].originalCmp = comparVal(first[i], pix1);
        second[i].originalCmp = comparVal(second[i], pix2);
    }


    RGB pRgb;
    pRgb.r = this->red;
    pRgb.b = this->blue;
    pRgb.b = this->blue;
    for(int i = 0; i < 100; i++)
    {

        first[i].parentCmp = comparVal(first[i], pRgb);
        second[i].parentCmp = comparVal(second[i], pRgb);
        first[i].averageCmp = (first[i].originalCmp+first[i].parentCmp)/2;
        second[i].averageCmp = (second[i].originalCmp+second[i].parentCmp)/2;
    }

    std::sort(first, first+100, [](const RGB& l, const RGB& r){return r.averageCmp > l.averageCmp;});
    std::sort(second, second+100, [](const RGB& l, const RGB& r){return r.averageCmp > l.averageCmp;});
    RGB bestfirst = first[rand()%10], bestsecond = second[rand()%10];

    DivisableSquare firstSQ(this->ParentContainter, this->parentImage);
    DivisableSquare secondSQ(this->ParentContainter, this->parentImage);
    firstSQ.init(bestfirst.r, bestfirst.g, bestfirst.b, width1, height1, posX1, posY1);
    secondSQ.init(bestsecond.r, bestsecond.g, bestsecond.b, width2, height2, posX2, posY2);
    firstSQ.Shape.setFillColor({(sf::Uint8)bestfirst.r, (sf::Uint8)bestfirst.g, (sf::Uint8)bestfirst.b});
    secondSQ.Shape.setFillColor({(sf::Uint8)bestsecond.r, (sf::Uint8)bestsecond.g, (sf::Uint8)bestsecond.b});
    firstSQ.Shape.setSize({(float)width1, (float)height1});
    secondSQ.Shape.setSize({(float)width2, (float)height2});
    firstSQ.Shape.setPosition({(float)posX1 + this->parentImage->getSize().x + 10, (float)posY1});
    secondSQ.Shape.setPosition({(float)posX2 + this->parentImage->getSize().x + 10, (float)posY2});
    this->ParentContainter->push_back(firstSQ);
    this->ParentContainter->push_back(secondSQ);
    //crash here

    for(unsigned int i = 0; i < this->ParentContainter->size(); i++)
    {
        if(this->ParentContainter->at(i).id == this->id)
            this->ParentContainter->erase(this->ParentContainter->begin()+i);
    }
}

I know this is a poor code but i just wanted to test things out, what could cause a vector::push_back to crash my app?

Answer 1

The problem is that you're adding to dSquares while executing code using a member of the vector. When the vector is resized (during the this->ParentContainter->push_back(firstSQ); call), the object that this points to is moved (since it is part of the vector). However, this keeps pointing at the previous location of the object, and when you try to push the second new square you access this deallocated memory, resulting in Undefined Behavior and (in this case) a crash.

A possible fix is to call dSquares.reserve(dSquares.size() + 2); before you call dSquares.at(rand() % dSquares.size()).Partup();. This will allocate extra memory of the (potential) two new objects that are added so that when you call push_back within Partup a reallocation of the vector will not occur.

Another possibility is to erase the parent square first, then push the two new squares to the vector. When you push the first new square, it won't have to resize the vector (since there will be space for at least one element from removing the parent). Pushing the second element might result in a resize, so dereferencing this after that push could still crash.