Search code examples
c++pointersmemoryvalgrindallocation

Valgrind error invalid read of size 4


void ChoreStack::add(int new_urgency_level, string new_job){
    for(int i = 0; i < length_; i++){
        Chore* temp_chore = chore_array_[i];
        if(temp_chore->level == new_urgency_level){
            temp_chore->joblist.append(new_job+"^");
        }
        chore_array_[i] = temp_chore;
        free(temp_chore);
    }
}

Hello, this is the part that Valgrind says "Invalid read of size 4". I have never learned anything related to memory allocation before. Could anyone please explain to me in details why there is a memory error and how to fix it? Thank you!

Here is my declaration:

class ChoreStack{
public:
    ChoreStack();
    ~ChoreStack();
    void initialize(int new_urgency_level);
    void add(int new_urgency_level, string new_chore);
    void erase(int erase_level);
    void print();
    void next();
int get_length();
private:
    struct Chore{
        int level;
        string joblist;
    };
    Chore** chore_array_;
    int length_;
    string* split_joblist(string joblist);
    int number_of_chores(string joblist);
};

ChoreStack::ChoreStack(): chore_array_(nullptr), length_(0){
}

ChoreStack::~ChoreStack(){
    delete[] chore_array_;
}

void ChoreStack::initialize(int new_urgency_level){
    delete[] chore_array_;
    chore_array_ = new Chore*[new_urgency_level];
    for(int i = 0; i < new_urgency_level; i++){
        Chore* temp_chore = new Chore;
        temp_chore->level = i+1;
        temp_chore->joblist = "";
        chore_array_[new_urgency_level-i-1] = temp_chore;
        delete temp_chore;
    }
    length_ = new_urgency_level;
    cout << "New priority list with levels 1-" << length_ << " initialized." << endl;
}

Here is the part related to ChoreStack::add() function in main():

int main(){
ChoreStack c;
string cmd_line;
string* cmd_ptr = new string[3];
bool initialized = false;
while(true){
    cout << "chores> ";
    getline(cin, cmd_line);
    int cmd_num = 0;
    if(cmd_line.find_first_not_of("ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz1234567890 ") != string::npos){
        cout << "Error: invalid input." << endl;
    } else {
        int begin_i = 0, space_occurence = 0;
        unsigned int i = 0;
        while(i <= cmd_line.length()){
            if(cmd_line[i] == ' ' || i == cmd_line.length()){
                space_occurence++;
                cmd_num++;
                if(space_occurence == 3){
                    cmd_ptr[cmd_num-1] = cmd_line.substr(begin_i);
                    break;
                } else {
                    cmd_ptr[cmd_num-1] = cmd_line.substr(begin_i, i - begin_i);
                }
                begin_i = i + 1;
            }
            i++;
        }
        string command = cmd_ptr[0];
        if(command == "init"){
            if(cmd_num == 1){
                cout << "Error: the number of priority levels is missing." << endl;
            } else if(cmd_num > 2){
                cout << "Error: too much input for initializing." << endl;
            } else {
                if(cmd_ptr[1].find_first_not_of("1234567890") != string::npos){
                    cout << "Error: the number of priority levels must be an integer larger than 0." << endl;
                } else if(stoi(cmd_ptr[1]) < 1){
                    cout << "Error: it is not possible to create a priority list with 0 or less levels." << endl;
                } else {
                    c.initialize(stoi(cmd_ptr[1]));
                    initialized = true;
                }
            }
        } else if(command == "add"){
            if(!initialized){
                cout << "Error: no priority list is initialized." << endl;
            } else {
                if(cmd_num == 1){
                    cout << "Error: priority level and chore description are missing." << endl;
                } else if(cmd_ptr[1].find_first_not_of("1234567890") != string::npos
                          || stoi(cmd_ptr[1])  < 1
                          || stoi(cmd_ptr[1]) > c.get_length()){
                    cout << "Error: priority level must be an integer between 1-3." << endl;
                } else if(cmd_num == 2){
                    cout << "Error: chore description is missing." << endl;
                } else {
                    c.add(stoi(cmd_ptr[1]), cmd_ptr[2]);
                }
            }
        }

Here is the error message:

invalid read of size 4:
 1.ChoreStack::add(int,std::__cxx11::basic_string<char,std::char_traits<char>,std::allocator<char>>)

 2.main

Address 0x5a9de70 is 0 bytes inside a block of size 40 free'd
 1. operator delete(void*)

 2. ChoreStack::initialize(int)

 3. main

Block was alloc'd at
 1. operator new(unsigned long)

 2. ChoreStack::initialize(int)

 3. main

and there are lot more errors in the same form of this one...


Solution

  • This is a classic access after free.

    void ChoreStack::initialize(int new_urgency_level){
        delete[] chore_array_;
        chore_array_ = new Chore*[new_urgency_level];
        for(int i = 0; i < new_urgency_level; i++){
            Chore* temp_chore = new Chore;
            temp_chore->level = i+1;
            temp_chore->joblist = "";
            chore_array_[new_urgency_level-i-1] = temp_chore;
            delete temp_chore;
        }
    

    Look closely at this code. You create a pointer called temp_chore. It points to an object you allocate with new. You then copy the value of temp_chore into chore_array_. So now the array has a pointer to the object you allocated with new.

    But then you delete the object you allocated. So now, chore_array_ has a pointer to an object you deleted. It is an error to attempt to dereference this pointer since the thing it points to no longer exists.

    But then in add, we have:

        Chore* temp_chore = chore_array_[i];
        if(temp_chore->level == new_urgency_level){
    

    So temp_chore->level is an attempt to access the level member of the object temp_chore points to. But you deleted that object.

            // Once you do this,
            chore_array_[new_urgency_level-i-1] = temp_chore;
    
            // this
            delete temp_chore;
    
            // does the same thing as this
            delete chore_array_[new_urgency_level-i-1];
    
            // because they're equal
    

    You will find things much easier if you keep collections of values rather than collections of raw pointers.