c++ segmentation fault when copying a matrix

Question

I'm having a problem with a matrix, and the problem is when I try to copy it it gives me the error 'Segmentation Fault'. This is the code involved:

main.cpp

void principal(Tauler &tauler) {
    GeneradorPuzzle puzzle;
    pilaMoviments pilaMovs;
    int nPuzzle, cont=0;
    bool valid;

    cout << "JOC DEL RUSH HOUR" << endl;
    cout << "ENTRA EL PUZZLE A JUGAR:" << endl;
    cin >> nPuzzle;
    valid=puzzle.esPuzzleValid(nPuzzle);
    if(!valid) {
        do {
            cout << "PUZZLE NO VALID. ENTRA EL PUZZLE A JUGAR:" << endl;
            cin >> nPuzzle;
            valid=puzzle.esPuzzleValid(nPuzzle);
        }while(!valid);
    }
    puzzle.posarPuzzleActiu(nPuzzle);
    tauler=Tauler(puzzle.midaPuzzle(),puzzle.totalVehicles());

    for(int i=0;i<puzzle.totalVehicles();i++) {
        Vehicle v(cont, puzzle.midaVehicle(i),puzzle.filaVehicle(i),puzzle.columnaVehicle(i),puzzle.direccioVehicle(i));
        valid=tauler.esValid(v);
        if(valid) {
            tauler.processar(v,cont);
            cont++;
        }
    }
}
int main() {
    pilaMoviments pilaMovs;
    Tauler tauler;
    char opcio;

    principal(tauler);
    tauler.mostrar();
    mostrarMenu();
    do {
        cout << "ENTRA OPCIO:" << endl;
        cin >> opcio;
        tractarOpcio(tauler,pilaMovs,opcio);
    }while(opcio!='X');
    return 0;
}

Tauler.cpp

Tauler::Tauler() {
    a_f=a_surt=a_n=a_valids=0;
    a_mp=NULL;
}
Tauler::Tauler(const Tauler &t) {
    a_f=t.a_f;
    a_n=t.a_n;
    a_surt=t.a_surt;
    a_valids=t.a_valids;
    reservarMemoria();
    copiar(t);
}
Tauler::Tauler(int nf, int nv) {
    a_f=nf;
    a_n=nv;
    a_v=new Vehicle[a_n];
    reservarMemoria();
    for(int j=0;j<a_f;j++)
        for(int i=0;i<a_f;i++)
            a_mp[i][j]='-';
}

Tauler::~Tauler() {
    alliberarMemoria();
}

// OPERADORS

Tauler& Tauler::operator=(const Tauler& y) {
    if (this!=&y) {
        alliberarMemoria();
        reservarMemoria();
        copiar(y);
    }
    return *this;
}

// METODES PRIVATS

void Tauler::copiar(const Tauler &t) {
    a_f=t.a_f;
    a_n=t.a_n;
    a_surt=t.a_surt;
    a_valids=t.a_valids;
    for(int i=0;i<a_f;i++) {
        for(int j=0;j<a_f;j++)
            a_mp[i][j]=t.a_mp[i][j];
    }
}
void Tauler::alliberarMemoria() {
    for(int i=0;i<a_f;i++)
        delete [] a_mp[i];  // s'alliberen les taules horitzontals
    delete [] a_mp;
}
void Tauler::reservarMemoria() {
    a_mp=new char*[a_f];
    for(int i=0;i<a_f;i++)
        a_mp[i]=new char[a_f];
}

Tauler.h

#ifndef TAULER_H
#define TAULER_H
#include "Vehicle.h"
#include "pilaMoviments.h"

class Tauler {
    // La classe que guardara la informacio del Tauler
    public:
        // CONSTRUCTORS I DESTRUCTOR
        Tauler();
        //Pre: --; Post: Posa Tauler per defecte.
        Tauler(int nf, int nv);
        //Pre: nf i nv entrats correctament. Post: Posa Tauler amb les files i el nombre de vehicles que li hem entrat.
        Tauler(const Tauler &t);
        //Pre: Tauler correcte. Post: Fa una copia de Tauler i li diu t.
        ~Tauler();
        //Pre: --; Post: Memoria alliberada.

        // OPERADOR
        Tauler &operator=(const Tauler &e);

        // CONSULTORS
        int Files() const;
        //Pre: Files del Tauler correctes. Post: Retorna les files i columnes del Tauler.
        int filaSurtida() const;
        //Pre: Files del Tauler correctes. Post: Retorna la fila de surtida del Tauler.
        int Vehicles() const;
        //Pre: a_valids del Tauler correctes. Post: Retorna el nombre de Vehicles valids del tauler.
        bool fiPartida() const;
        //Pre: Vehicle 'A' al tauler. Post: Retorna true si el Vehicle 'A' esta a la ultima columa de la fila de surtida, altrament retorna false.
        bool esValid(Vehicle v) const;
        //Pre: Parametres del Vehicle v entrats correctament. Post: Retorna true si el vehicle es pot posar correctament dins el tauler. False si no es pot posar.
        bool xoquen(Vehicle v, Moviment m) const;
        //Pre: Parametres del Vehicle v, files i cols entrats correctament. Post: Retorna true si no hi ha cap altre vehicle bloquegi el desplaçament. Si n'hi ha algun retorna false.
        void mostrar() const;
        //Pre: Tauler ple. Post: Mostra per pantalla totes les posicions del tauler amb els vehicles.

        // MODIFICADORS
        void posarVehicle(char a, Vehicle v);
        //Pre: a correcte i Vehicle v valid. Post: Coloca el vehicle del puzzle al Tauler al lloc que li toca.
        void processar(Vehicle v, int cont);
        //Pre: Vehicle v valid i cont>=0. Post: Assigna una lletra al vehicle i si es el primer horitzontal guarda la fila com a fila de surtida.
        bool validarMov(Moviment m);
        //Pre: --; Post: Retorna false si la lletra no es de cap Vehicle del taulell, o si el moviment no es pot fer, altrament retorna true.
        void ferMov(pilaMoviments pilaMovs, Moviment m);
        //Pre: Parametres lletra, files i cols correctes. Post: Mou el vehicle que tingui la lletra entrada les files i columnes que ens entren i empila el moviment a la pila.
        void desferMov(pilaMoviments pilaMovs);
        //Pre: Movimetns de la pila >0. (No es pot desfer moviments si no n'hi ha cap). Post: Desfa l'ultim moviment i el desempila de la Pila de moviments.
        bool movPossible(Vehicle v, int files, int cols);

    private:
        // TAULA DE VEHICLES
        Vehicle * a_v;
        int a_valids;

        // ATRIBUTS
        pilaMoviments a_pila;
        int a_f;
        int a_n;
        int a_surt;
        char ** a_mp;

        // METODES
        void alliberarMemoria();
        void reservarMemoria();
        void copiar(const Tauler &t);
};

#endif // TAULER_H

When I start it, it asks for a puzzle as it should, but then it crashes in this line:

a_mp[i][j]=t.a_mp[i][j];

In the 'copiar' method. What I undertand is that I'm giving a_mp[i][j] a value that I don't know, but I don't know how to fix it, any ideas? I know the names aren't in English but I hope this isn't a problem.

Thanks a lot EDIT: Deleted iniciar(), it wasn't the cause of the problem

That code is a pain to read, but are you even making sure that a_mp is initialized to the proper size? You make it a lot easier for people to read the code if you minimize the code needed to reproduce the issue. — SinisterMJ, May 02 '13 at 12:09
This code `Tauler& Tauler::operator=(const Tauler& y) { ... y.iniciar(); ... }` should not compile. — john, May 02 '13 at 12:12
Can you show the stack trace of that seg fault? I don't even know on what path the error occurs, which makes this ridiculous hard to read. — SinisterMJ, May 02 '13 at 12:15
[Make a testcase](http://sscce.org). You should already have done this, as part of your own debugging. — Lightness Races in Orbit, May 02 '13 at 12:16
Not sure what you mean with this, but what it does untill it crashes is: principal(tauler); in (int main), tauler=Tauler(puzzle.midaPuzzle(),puzzle.totalVehicles()); in (void principal) copiar(y); in the = operator and a_mp[i][j]=t.a_mp[i][j]; in the copy method. @john, I only introduced that method to test something but it didn't work so I deleted it. That's not causing the error. — p. bosch, May 02 '13 at 12:18

score 3 · Answer 1 · answered May 02 '13 at 12:19

I think you made a mistake here:

Tauler& Tauler::operator=(const Tauler& y) {
if (this!=&y) {
    alliberarMemoria();
    reservarMemoria();
>>     y.iniciar();
    copiar(y);
}

If an instance of your object has a a_f that's less than y.a_f then when you're copying the y you're writing to places that's not allocated. You probably want to do iniciar(), and not y.iniciar().

I might be wrong, since you haven't posted iniciar() source code.

score 2 · Accepted Answer · answered May 02 '13 at 12:14

2

It looks like operator= starts by freeing current memory (ok), allocating memory (but how much should be allocated?) and copy.

If the source size is bigger than destination size you are going to overwrite memory that was not allocated.

Before calling the memory allocator you should first set a_f and a_n to the correct values.

answered May 02 '13 at 12:14

6502

112,025
15
165
265

Since I have to make a copy of the current matrix, is this what you mean? `Tauler& Tauler::operator=(const Tauler& y) { if (this!=&y) { alliberarMemoria(); y.a_f=a_f; y.a_n=a_n; reservarMemoria(); copiar(y); } return *this; }` – p. bosch May 02 '13 at 12:23
@p.bosch: no, exactly the opposite: in the body you should do `a_f=y.a_f;` and `a_n=y.a_n;` before calling `reservarMemoria`. You are not making a copy of `*this`; you are copying `y`. – 6502 May 02 '13 at 12:42
I can't do that, it says: 'assignment of member 'Tauler::a_f' in read-only object' – p. bosch May 02 '13 at 12:50

score 1 · Answer 3 · edited May 23 '17 at 11:57

1

Not the answer, but a couple of suggestions to find it.

First, isolate in your code reading the value and writing to the other matrix, i.e. isolate the query from the command.

Instead of

a_mp[i][j]=t.a_mp[i][j];

Write:

int valorAAssignar = t.a_mp[i][j];
a_mp[i][j] = valorAAssignar;

Second, assert that all your array indirections are between their limits.

assert(0 <= i);
assert(i <= SOME_MAXIMUM_VALUE_FOR_I);
assert(0 <= j);
assert(j <= SOME_MAXIMUM_VALUE_FOR_J);
int valorAAssignar = t.a_mp[i][j];
a_mp[i][j] = valorAAssignar;

Do the same at all other points where there is an array indirection. Actually, you'll be better off writing a method such as ReadCellValue, or in Catalan LlegeixValor:

   LlegeixValor(a_mp, i, j);

Which basically does what I suggested above: check the limits and return the value.

You should find what's going wrong if you run it in debug mode, where assertions are checked. Mare sure of this last point by writing assert(false); and seeing that it fails. Then remove this line.

This code cries for preconditions, postconditions and class invariants to be added (see Design by Contract). Maybe if you're lucky and the failure is simple you can get by just with precondition assertions, such as the ones I suggested.

edited May 23 '17 at 11:57

Community

1
1

answered May 02 '13 at 12:20

Daniel Daranas

22,454
9
63
116

Sorry but i don't understand what you want me to do with `assert`. I have never used it before so I don't think I should use it in here. This is the final project of a subject in the university and they never said anything about assert. Is there any other way? Oh and if I use your first advice, it crashes in the second line: `a_mp[i][j] = valorAAssignar;` – p. bosch May 02 '13 at 12:40
(1) C++ is a language, and assert has a meaning in C++. Even though it is a macro and not a function, it is an essential tool. I'm telling you to check array bounds. If you don't know assert you can do it with an "if" instruction. It's an inferior solution but it's another way to investigate why your code crashes. With a simple "if " you can check if your array indexes are valid. I think they will let you do this at university. – Daniel Daranas May 02 '13 at 12:43
(2) "If I use your first advice, it crashes in the second line: a_mp[i][j] = valorAAssignar;" Great, you have now simplified your problem in a 50%. You know the reading part did not crash, and the writing fails. Dividing a problem is another way of finding its root cause. – Daniel Daranas May 02 '13 at 12:44
(1.1) More on the assert macro: http://stackoverflow.com/questions/1571340/what-is-the-assert-function Note also that once you've found what the problem actually is and your project is finished, you can freely delete all your assert instructions and the people at university will not see them or know that they ever existed. assert doesn't do anything in release mode, it's "just" a testing, debugging and built-in documentation tool. It doesn't do any effective job in your program, it just checks its correctness. – Daniel Daranas May 02 '13 at 12:51

c++ segmentation fault when copying a matrix

3 Answers3