Determining why C++ program chrashes

Question

I'm doing a C++ project for a PCB assembly thesis and I have been given (by my professor) an old set of C++ code. When I try test and run the code it chrashes...the program compiles ok, but it chrashes at runtime..here is the code:

main.cpp:

#include <iostream>
#include <vector>
#include <string>
#include <cstdlib>
#include <ctime>
#include <climits>
#include "NozzleBank.h"
#include "PlacementHead.h"

int main (int argc, char * const argv[]) {
    std::vector<char> list;
    for (int i=0; i<3; i++) {list.push_back('a');}
    for (int i=0; i<3; i++) {list.push_back('b');}
    for (int i=0; i<3; i++) {list.push_back('c');}
    for (int i=0; i<3; i++) {list.push_back('d');}
    for (int i=0; i<3; i++) {list.push_back('_');}

    int i = 0;
    char set[list.size()];
    while (!list.empty()) {
        int x = (rand() % list.size());
        set[i] = list.at(x);
        list.erase(list.begin()+x);
        i++;
    }

    NozzleBank bank(15,set);
    PlacementHead head(4,2,1,"abababab");

    return 0;
} 

PlacementHead.cpp:

#include "PlacementHead.h"
#include <string>
#include <iostream>
#include <string.h>

PlacementHead::PlacementHead(int width, int height, int gap, char* s) {
    width_ = width;
    height_ = height;
    gap_ = gap;
    size_ = (width*height)+1;
    set_ = new char[size_];
    from_ = new int[size_];
    original_ = new char[size_];
    strcpy(set_,s);
    strcpy(original_,s);
}

PlacementHead::~PlacementHead() {

}

int PlacementHead::getSize() { return size_; }
int PlacementHead::getHeight() { return height_; }
int PlacementHead::getWidth() { return width_; }
int PlacementHead::getGap() { return gap_; }

// Palauttaa indeksissä i olevan suuttimen
char PlacementHead::getNozzle(int i) {
    return set_[i-1];
}

// Asettaa indeksissä i olevan suuttimen
void PlacementHead::setNozzle(int i, char c) {
    set_[i-1] = c;
}

// Merkitsee suuttimen poimituksi poistamalla sen listasta
void PlacementHead::markNozzle(int i, int bankPos) {
    set_[i-1] = ' ';
    from_[i-1] = bankPos;
}

// Palauttaa seuraavan poimimattoman suuttimen indeksin
int PlacementHead::getNextUnmarkedPos() {
    for (int i=0; i<size_; i++) {
        if (set_[i]!=' ') {
            return i+1;
        }
    }
    return 0;
}

// Palauttaa suuttimen alkuperäisen sijainnin pankissa
int PlacementHead::getBankPos(int i) {
    return from_[i-1];
}

// Plauttaa alkuperäisen ladontapaan suutinjärjestyksen
void PlacementHead::reset() {
    //for (int i=0; i<size_; i++) {
    //  set_[i] = original_[i];
    //}
    strcpy(set_,original_);
}

// Tulostusmetodi
void PlacementHead::print() {
    std::cout << "ladontapää:\n";
    for (int h=height_; h>0; h--) {
        for (int w=width_; w>0; w--) {
            int i = ((h-1)*width_)+w;
            std::cout << getNozzle(i);
        }
        std::cout << "\n";
    }
}

NozzleBank.cpp:

#include "NozzleBank.h"
#include <string>
#include <iostream>
#include <string.h>

NozzleBank::NozzleBank(int size) {
    bank_ = new char[size];
    original_ = new char[size];
    size_=size;
    for (int i=0; i<size_; i++) {
        bank_[i] = ' ';
        original_[i] = ' ';
    }
}

NozzleBank::NozzleBank(int size, char* s) {
    bank_ = new char[size];
    original_ = new char[size];
    size_ = size;
    strcpy(bank_,s);
    strcpy(original_,s);
}

NozzleBank::~NozzleBank() {

}

int NozzleBank::getSize() { return size_; }

// Palauttaa pankin alkuperäisen järjestyksen
void NozzleBank::reset() {
    strcpy(bank_,original_);
}

// Asettaa indeksissä i olevan suuttimen
void NozzleBank::setNozzle(int i, char c) {
    bank_[i-1] = c;
    original_[i-1] = c;
}

// Palauttaa indeksissä i olevan suuttimen
char NozzleBank::getNozzle(int i) {
    return bank_[i-1];
}

// Poimii suuttimen poistamalla sen listasta
void NozzleBank::pickNozzle(int i) {
    bank_[i-1] = ' ';
}

// Tulostusmetodi
void NozzleBank::print() {
    for (int i=size_; i>0; i--) {
        std::cout << bank_[i-1];
    }
}

When I run the program I get the following:

Now here is also an interesting thing: IF I switch the order of the following lines in main.cpp

NozzleBank bank(15,set);
PlacementHead head(4,2,1,"abababab");

to:

PlacementHead head(4,2,1,"abababab");
NozzleBank bank(15,set);

The program runs fine...:O? And here I get like whaaaat...I'm a bit newbie in C++ so I'd appreciate if someone could see what's the problem :) Thank you for any help!

Answer 1

One of the possible problems here is that you are using strcpy

strcpy works by reading an array of characters until it reaches a null terminating character '\0' - However your source array does not contain a null terminating character. so strcpy will keep copying forever, reading memory it doesn't have access to, and writing past the end of your destination array, either of which could cause the crash. You need to either use strncpy (which you should always prefer), which only copies a fixed number of characters.

You should in general always leave an extra space in any character array if you intend on treating the characters as a string, like strcpy does. If you are only using individual elements and treating the individual characters on their own, then you don't need to. You could equally use memcpy in that case.

There may be other problems in the code, this is just one I have spotted.

You also have memory leaks, you should delete[] the member variables you new[]

Answer 2

The C library strcpy() requires a NUL terminated C style string. You have a char[] array that does not have a NUL terminator.

If you want to convert your randomized array of letters into a C-style string, you need to add one more element to the end, and set it to the value 0. Alternately, you need to turn your strcpy() calls into memcpy() calls and supply the length directly.

In terms of minimal code changes, adding a NUL terminator requires the fewest changes to your code. Change this:

    char set[list.size()];

to this:

    char set[list.size() + 1];
    set[list.size()] = 0;

And then change all of your new char[size_] calls to new char[size_ + 1].

The cleaner approach would be to say what you mean, and treat this as an array of char, not a C string. Convert all your strcpy calls to memcpy. For example, this:

    strcpy(set_,s);
    strcpy(original_,s);

becomes this:

    memcpy(set_,s,size_);
    memcpy(original_,s,size_);

Note: Be sure to change all strcpy to memcpy on these arrays. I think there is at least one other. I did not check your code that closely.

The latter is the better approach, IMHO, but I offer both.

Also, the above code leaks memory. That may not be a concern if it just runs through once and quits. You new [] memory that you never delete []. If your program is going to grow and have multiple instances of these objects coming and going, you'll want to add those delete[] calls, otherwise you're setting yourself up for a future crash of a different sort.