Search code examples
c++filefstream

Can't write/read an array of class objects to a file


I have a class User with 3 parameters: id, pass, active

I want to write an array of User objects to a file but it is not work.

Sometimes it is work but after that I can read only these parameters: id, active.

The parameter pass gives me: nothing, some characters or one character(almost 'a').

The class:

class User {
    int id;
    char *pass;
    bool active;
public:
    User() {
        id = -1;
        pass = NULL;
        active = 0;
    }
    User(int id, char * pass, bool active = 1) {
        this->id = id;

        if (pass) delete[] pass;
        this->pass = new char[strlen(pass)];
        memcpy(this->pass, pass, strlen(this->pass));

        this->active = active;
    }

    void set_id(int id) {
        this->id = id;
    }
    void set_active(bool active) {
        this->active = active;
    }
    void set_pass(char *pass) {
        this->pass = new char[strlen(pass)];
        memcpy(this->pass, pass, strlen(this->pass));
    }

    int get_id() {
        return id;
    }
    char *get_pass() {
        return pass;
    }
    bool get_active() {
        return active;
    }
};

It is my array:

User *users = new User[999999]; int size_ = 0;

Main:

int main() {

    fstream r_user("user.txt", ios::binary | ios::in);
    if (!r_user) {
        r_user.close();
        char pass[100];
        cerr << "You don't have any user, please create user" << endl;
        cout << "User pass: "; cin >> pass;
        users[0].set_id(0);
        users[0].set_pass(pass);
        users[0].set_active(1);
        size_++;
        cout << users[0].get_pass();
        fstream w_user("user.txt", ios::binary | ios::out);
        w_user.write(reinterpret_cast<char *>(users), size_ * sizeof(User));
        w_user.close();
    }
    else {
        r_user.seekg(0, r_user.end);
        size_ = r_user.tellg();
        r_user.seekg(0, r_user.beg);
        size_ = size_ / sizeof(users);
        r_user.read(reinterpret_cast<char *>(users),  99999 * sizeof(User));
        r_user.close();
        cout << users[0].get_pass();
    }
    return 0;
}

The file is not empty.


Solution

  • Your code suffers two critical bug in the way you handle the storage of the pass.

    You use:

        this->pass = new char[strlen(pass)];
        memcpy(this->pass, pass, strlen(this->pass));
    

    Note that any string is NULL byte terminated, so the size of the data you want to copy are strlen(pass)+1. Eg: strlen("abcedf") == 6 but you need 7 bytes to store it.

    Consequences can be dramatic: when trying to read the string, the software will hit other chunks of memory concatenated to this region, and will read an erroneous string (because no NULL byte stops the string reading). Over time, with more functions, this small error may leak memory into files, might cause a crash or write NULL bytes in an off-by-one fashion, functions stop working properly and everything goes downhill... Sounds like hell uh?

        this->pass = new char[strlen(pass) + 1];
        memcpy(this->pass, pass, strlen(this->pass) + 1);
    

    Or, more elegant:

        this->pass = new char[strlen(pass) + 1];
        this->pass = strcpy(this->pass, pass);
    

    Or more elegant and safer:

        this->pass = strdup(pass);
    

    You also might want to use a unique private function for this kind of management, actually this sensitive part is duplicated in your code.

    And hash the pass maybe? :-) Go for Bcrypt if you want something serious for the job: https://github.com/rg3/bcrypt

    I also see that you use a pointer for that pass stuff. So you save the pointer, but not the pointed data.

    When loading the value of the pointer, nothing is there in memory, or if something is there it's certainly not your pass (which have never been saved). You must use a static buffer to store that pass, and ensure that your class will stay POD (Plain Old Data).

    With the use of hashed passwords, your final buffer will have a deterministic size. Eg:

    char bcrypt_password[64];
    

    instead of:

    char *pass;
    

    But then beware your allocation:

     new User[999999];
    

    will eat up fair chunks of memory.

    I would be you, I would go for a serialization of your object, and the integration of import() export() methods. A basic CSV serialization would allow for later DB imports / exports.