C++ passing vector<string> element to function causes SIGSEGV crash

Question

The problem is, as the title says, when I try to pass the second vector element to the function "solveSide".This vector contains two string tokens, obtained from a bigger string with a user defined function.

The program works like this:

• read line from file into a string
• call "stringSplit" function to obtain two tokens from that line string, and put them into a vector
• call "solveSide" for the first element in the vector
• call "solveSide" for the second element in the vector

The problem appears in the second call to "solveSide", as the program crashes before entering the function body.

If I delete the second call to the "solveSide" function, the program runs just fine. If I try to output the two elements of vector without deleting that line it doesn't ouptut anything, but with it deleted, it outputs both elements.

In debug mode it throws SIGSEGV, when it calls "std::string::_M_mutate(unsigned int, unsigned int, unsigned int)" (as shown by the call stack).

Crash Code:

ifstream fin("ecuatii.in");
string ecuatie;
vector<string> sideSplit;

fin>>ecuatie;
sideSplit = stringSplit(ecuatie, "=");
fout<<sideSplit[0]<<"="<<sideSplit[1]<<"\n";
solveSide(sideSplit[0], leftCoeffSum, leftSum);
solveSide(sideSplit[1], rightCoeffSum, rightSum);

stringSplit function:

vector<string> stringSplit(string str, const char *delim){
    char *strCopy = new char [str.size()+1];
    char *token;
    char **tokens = new char* [(str.size()+1)*2];
    unsigned tokenCount = 0;

    strcpy(strCopy, str.c_str());
    token = strtok(strCopy, delim);
    while(token != NULL){
        tokens[tokenCount++] = token;
        token = strtok(NULL, delim);
    }

    vector<string> splits(tokenCount);
    for(unsigned i = 0; i < tokenCount; ++i){
         splits[i] = "";
         splits[i] += tokens[i];
    }

    delete[] strCopy;
    for(unsigned i = 0; i < tokenCount; ++i){
         delete[] tokens[i];
    }
    delete[] tokens;

    return splits;
}

stringChunkToFloat function(should've used "substr()", now I know):

float stringChunkToFloat(string str, unsigned posStart, unsigned posEnd){
    char numberStr[20];
    for(unsigned i = posStart; i <= posEnd; ++i){
        numberStr[i-posStart] = str[i];
    }
    numberStr[posEnd-posStart+1] = '\0';

    float number = atof(numberStr);

    return number;

}

solveSide function:

void solveSide(string &side, float &coeffSum, float &sum){
    coeffSum = 0;
    sum = 0;

    unsigned posStart = side.find_first_of("+-", 1);
    unsigned posEnd;
    float currentNumber;

    if(side.size() == 1){
        if(side[0] == 'x'){
            coeffSum = 1;
        }else{
            sum = atof(side.c_str());
        }
        return;
    }

    if(side[0] == 'x'){
        coeffSum += 1;
    }else if(side[1] == 'x' && side[0] == '-'){
        coeffSum -= 1;
    }else if(side[posStart-1] == 'x'){
        currentNumber = stringChunkToFloat(side, 0, posStart-2);
        coeffSum += currentNumber;
    }else{
        currentNumber = stringChunkToFloat(side, 0, posStart-1);
        sum += currentNumber;
    }

    while(posStart != string::npos && posStart != side.size()){
        posEnd = side.find_first_of("+-", posStart+1);
        if(posEnd == string::npos){
            posEnd = side.size();
        }

        if(side[posStart+1] == 'x'){
            if(side[posStart] == '+'){
                coeffSum += 1;
            }else{
                coeffSum -= 1;
            }
        }else if(side[posEnd-1] == 'x'){
            currentNumber = stringChunkToFloat(side, posStart, posEnd-2);
            coeffSum += currentNumber;
        }else{
            currentNumber = stringChunkToFloat(side, posStart, posEnd-1);
            sum += currentNumber;
        }

        posStart = posEnd;
    }
}

Input string : 2.0x-4+5x+300=98x

Answer 1

I found the problem, it's more of a thinking error:

When the function solveSide is called with the second string as an argument which has the value 98x, it searches for "+" or "-" sign, after the search startPos = npos because it does't find it, and it enters this case, just before the while:

}else{
    currentNumber = stringChunkToFloat(side, 0, posStart-1);
    sum += currentNumber;
}

When it tries to get the number from position 0 to npos, it throws the SIGSEGV (Segmentation Fault), because it tries to acces unallocated memory from the string passed to stringChunkToFloat

Answer 2

This block is not only unnecessary, but brings you the joy of undefined behaviour:

for(unsigned i = 0; i < tokenCount; ++i){
     delete[] tokens[i];
}

Think about it: you allocated memory for tokens, not for the individual tokens themselves. After all, they are pointers into your tokenized string (std::strtok is destructive).

Even better, don't use raw memory at all if you can avoid it:

vector<string> stringSplit(const string& str, const char *delim){
    // Note: if you use C++11, you can use std::string::data() 
    // and don't need a copy to a vector, just take the string by value
    vector<char> strCopy(str.begin(), str.end());
    strCopy.push_back('\0');

    vector<char*> tokens;
    {        
        char * token = strtok(strCopy.data(), delim);
        while(token != NULL){
            tokens.push_back(token);
            token = strtok(NULL, delim);
        }
    }

    vector<string> splits(tokens.size();
    for(unsigned i = 0; i < tokens.size(); ++i){
         splits[i] = "";
         splits[i] += tokens[i];
    }

    return splits;
}

Answer 3

Its more safety way working with vector is doing splits.pushback() method and size more safety get as spits.size(). Always checking vector.size() before assignment to vector' member.