[C++] *** glibc detected *** double free or corruption

[Anoniem: 220084]

Hallo,

Sinds kort volg ik uit interesse een vak over Computer Graphics. Voor een opdracht moet ik een programma schrijven met Qt waarin een bol, een pyramide en een kubus voorkomen. Het idee is om in plaats van glTranslate, glRotate etc een eigen math library te gebruiken.

Ik heb ervoor gekozen een Matrix class te maken die in de math library gebruikt wordt. Deze class bevat een 1dimensionale array die op de heap geallocate wordt. Door de haakjes () te overloaden wordt het gebruiksgemak van 2dimensionale arrays verkregen. Stel dat we een Matrix genaamd 'mat' hebben, dan kan je bijvoorbeeld schrijven 'mat(1,2) = 31;' om maar wat te noemen. Dit maakt het schrijven van functies voor bijvoorbeeld de inverse van een matrix een stuk makkelijker.

Het programma is gecompiled met gcc 4.4.1 en Qt 4.5 in ubuntu 9.10. Er worden geen foutmeldingen gegeven tijdens het compilen. Via de IDE Eclipse heb ik een debug gedaan. In een van de functies van de math library wordt een Matrix object op de stack aangemaakt. De debugger blijft net hangen na deze functie en het programma crasht. Als het object buiten de scope valt wanneer de functie verlaten wordt, dan wordt de destructor zoals verwacht aangeroepen lijkt het. Dan crasht het programma met de volgende melding: " *** glibc detected *** double free or corruption (fasttop)". Het vrijmaken van de 'heap allocated array' van het Matrix object in de destructor is toch wel echt essentieel. Als ik de destructor van de class Matrix leeg laat dan draait het programma prima, maar is er een fors geheugenlek zoals verwacht. Ik heb de Matrix class hieronder weergegeven samen met een screenshot van de Eclipse debugger waarin het probleem ook naar voren komt.

Kunnen jullie me misschien vertellen wat ik fout doe ? Ik ben nog niet zo lang met C++ bezig dus misschien zie ik wat over het hoofd. Het probleem zit hem in dit deel van de code, omdat als ik de destructor leegmaak er geen enkel probleem is (behalve de memory leak dan).

mvg, Bram

code:

template <class T>
 class Matrix {
    public:
     T* data; //the only reason that it is public is that it can be loaded into glLoadMatrix.

     Matrix(unsigned r, unsigned c)
     {
         rows = r;
         cols = c;
         data = new T[rows * cols];
     }

     ~Matrix()
     {
         delete[] data; // to prevent memory leak
     }

     T& operator() (unsigned i, unsigned j)
     {
         return data[i + rows * j]; //internally represent [i][j] as [j][i] when flattening 2D array into 1D array because glLoadMatrix loads column after column instead of row after row
     }

     T operator() (unsigned i, unsigned j) const
     {
         return data[i + rows * j];
     }

     void loadIdentity(){ //assuming square matrix
        for (unsigned i = 0; i < rows; i++){
            for (unsigned j = 0; j < cols; j++){
                if (i != j)
                {
                    data[j*rows + i] = 0;
                }
                else
                {
                    data[j*rows + i] = 1;
                }
            }
        }
     }


    private:
      unsigned rows, cols;

    };

[Soultaker]

In de code die je hier laat zien zit de fout niet, maar ik vermoed dat er kopiën worden gemaakt van een matrix. Aangezien je geen copy constructor of assignment operator hebt gedefinieerd, zal een standaardimplementatie gebruikt worden, waarbij velden één op één gekopieerd worden. Dat gaat hier mis, omdat de pointer naar de matrixdata dan ook gekopieerd wordt, zonder nieuw geheugen te alloceren. Vervolgens wordt de destructor voor zowel het oorspronkelijke object als de kopie aangeroepen, en wordt twee keer dezelde array vrijgegeven.

Denk er aan dat als je een Matrix object by value doorgeeft aan een functie, dus niet via een pointer of reference, er ook een kopie gemaakt wordt. Zoiets gaat dus fout:

C++:

void foo(Matrix<int> a) {
    // a.data == b.data (maar dat wilde je niet)
    // a's destructor geeft hier a.data vrij!
}

int main() {
    Matrix<int> b(12, 34);
    foo(b);
    // b's destructor probeert hier nogmaals b.data vrij te geven!
}

De oplossing is dus om een eigen copy constructor en assignment operator te definiëren (het is meestal een goed idee om niet één van de twee, maar allebei te definiëren) die expliciet geheugen alloceert voor het nieuwe object en dan de inhoud kopieert.

[ Voor 18% gewijzigd door Soultaker op 20-01-2010 17:26 ]

[.oisyn]

De rule of thumb is hier dat als je een dtor implementeert, je over het algemeen óók een copy ctor en copy assignment operator moet implementeren óf je object noncopyable maken (door de copy ctor en copy assignment operator private te declareren en verder niet te implementeren)

[Anoniem: 220084]

Bedankt voor de tip! Ik dacht dat als je een object naar een functie stuurde je gewoon met hetzelfde object verder werkte. Een soort pass by reference dus. Een kopie van het object maken is niet gewenst in dit geval. Ik heb net mn functies aangepast door ze objecten als reference aan te laten nemen. Nu crasht het programma niet direct meer, maar krijg ik na een paar frames een segmentation fault. Deze segmentation fault had ik niet hiervoor (toen ik het programma liet draaien door de destructor gewoon leeg te laten, memory leak verhaal van hiervoor). Dus blijkbaar gaat er iets fout in het geheugen nu ik de objecten als reference meestuur. Maar daar kunnen jullie verder niet zo veel mee helpen denk ik. nogmaals bedankt voor de tip

Soultaker schreef op woensdag 20 januari 2010 @ 17:22:
In de code die je hier laat zien zit de fout niet, maar ik vermoed dat er kopiën worden gemaakt van een matrix. Aangezien je geen copy constructor of assignment operator hebt gedefinieerd, zal een standaardimplementatie gebruikt worden, waarbij velden één op één gekopieerd worden. Dat gaat hier mis, omdat de pointer naar de matrixdata dan ook gekopieerd wordt, zonder nieuw geheugen te alloceren. Vervolgens wordt de destructor voor zowel het oorspronkelijke object als de kopie aangeroepen, en wordt twee keer dezelde array vrijgegeven.

Denk er aan dat als je een Matrix object by value doorgeeft aan een functie, dus niet via een pointer of reference, er ook een kopie gemaakt wordt. Zoiets gaat dus fout:

C++:

void foo(Matrix<int> a) {
    // a.data == b.data (maar dat wilde je niet)
    // a's destructor geeft hier a.data vrij!
}

int main() {
    Matrix<int> b(12, 34);
    foo(b);
    // b's destructor probeert hier nogmaals b.data vrij te geven!
}

De oplossing is dus om een eigen copy constructor en assignment operator te definiëren (het is meestal een goed idee om niet één van de twee, maar allebei te definiëren) die expliciet geheugen alloceert voor het nieuwe object en dan de inhoud kopieert.

[Soultaker]

Neem ook even .oisyn's tip mee: als kopiëren niet de bedoeling is, declareer de copy constructor en assignment operator dan private:

C++:

class Matrix {
// ...
private:
  Matrix(const Matrix &);  // copy constructor
  Matrix &operator=(const Matrix &); // assignment operator
}

Je hoeft ze dan verder niet te definiëren, maar je voorkomt hiermee dat ze per ongeluk aangeroepen worden.

[ Voor 13% gewijzigd door Soultaker op 20-01-2010 19:49 ]

[Anoniem: 220084]

Soultaker schreef op woensdag 20 januari 2010 @ 19:48:
Neem ook even .oisyn's tip mee: als kopiëren niet de bedoeling is, declareer de copy constructor en assignment operator dan private:

C++:

class Matrix {
// ...
private:
  Matrix(const Matrix &);  // copy constructor
  Matrix &operator=(const Matrix &); // assignment operator
}

Je hoeft ze dan verder niet te definiëren, maar je voorkomt hiermee dat ze per ongeluk aangeroepen worden.

fantastisch! De compiler kon direct het probleem achterhalen door deze suggestie. Ik was een debugfunctie vergeten aan te passen om 'Matrix' als reference aan te nemen waardoor er juist bugs ontstonden... Erg bedankt, Ik heb me heel lang lopen blind staren op dit probleem.

[muksie]

C++:

T* data; //the only reason that it is public is that it can be loaded into glLoadMatrix.

Is het niet beter om deze protected/private te maken en een public T* getData() oid toe te voegen?

[Zoijar]

muksie schreef op woensdag 20 januari 2010 @ 21:08:

C++:

T* data; //the only reason that it is public is that it can be loaded into glLoadMatrix.

Is het niet beter om deze protected/private te maken en een public T* getData() oid toe te voegen?

Meestal wel. Of een cast operator naar T* (dan kan je glLoadMatrix() gewoon direct aanroepen met een matrix)

[ Voor 8% gewijzigd door Zoijar op 21-01-2010 07:48 ]

[Anoniem: 84120]

Als we dan toch commentaar gaan geven; initializer list, const voor rowsen cols (ik zie ze nergens veranderen); iets om rows en cols op te vragen. Zo is er vanalles te verfraaien (en dan nog een hele rits matrixoperatie implementeren natuurlijk).

[ Voor 15% gewijzigd door Anoniem: 84120 op 21-01-2010 10:16 ]

[ATS]

Nog een tip: mocht je willen dat je je matrices wel gewoon by value kan doorgeven, kijk dan eens naar wat Qt's shared data classes je te bieden hebben. Begin bijvoorbeeld bij QSharedDataPointer. Die zorgen dat je eenvoudig een implementatie kan maken waarbij standaard alleen een pointer naar je data gecopieerd wordt, en de data zelf pas gecopieerd wordt als je er naar probeert te schrijven ("copy-on-write" principe). Qt gebruikt het zelf ook op heel veel plaatsen, zoals in QString.

[.oisyn]

Ik ben nooit zo van de copy-on-write semantics. Het impliceert namelijk dat je reads ook moet gaan synchroniseren. Dat lijkt me bij een matrix klasse minder handig ivm performance.