C++ object that modifies itself in memory

Question

A friend of mine and some other guy have written the following code that according to my C++ knowledge should be very dangerous:

Recovery& Recovery::LoadRecoverFile() {
    fstream File("files/recover.dat", ios::in | ios::binary);
    if (File.is_open()) {

        while (!File.eof()) {
            File.read(reinterpret_cast<char*>(this), sizeof(Recovery));  // <----- ?
        }
    }
    File.close();
    return *this; // <----- ?
}

Could you give me your opinion why this is bad and how should it be done correctly?

They basically write an object of class Recovery to a file and when required they read it in with the above method.

Edit: Just to give some additional information about the code. This is what class Recovery contains.

class Recovery {
public:
    Recovery();
    virtual ~Recovery();
    void createRecoverFile();
    void saveRecoverFile( int level, int win, int credit, gameStates state, int clicks );
    Recovery& LoadRecoverFile();
    const vector<Card>& getRecoverCards() const;
    void setRecoverCards(const vector<Card>& recoverCards);
    int getRecoverClicks() const;
    void setRecoverClicks(int recoverClicks);
    int getRecoverCredit() const;
    void setRecoverCredit(int recoverCredit);
    int getRecoverLevel() const;
    void setRecoverLevel(int recoverLevel);
    gameStates getRecoverState() const;
    void setRecoverState(gameStates recoverState);
    int getRecoverTime() const;
    void setRecoverTime(int recoverTime);
    int getRecoverWin() const;
    void setRecoverWin(int recoverWin);

private:
    int m_RecoverLevel;
    int m_RecoverCredit;
    gameStates m_RecoverState;
};

This saves the object to a file:

void Recovery::saveRecoverFile(int level, int win, int credit, gameStates state,
        int clicks) {

    m_RecoverLevel = level;
    m_RecoverCredit = credit;
    m_RecoverState = state;

    ofstream newFile("files/recover.dat", ios::binary | ios::out);
    if (newFile.is_open()) {
        newFile.write(reinterpret_cast<char*>(this), sizeof(Recovery));
    }

    newFile.close();
}

That's how it is used:

    m_Recovery.LoadRecoverFile();
    credit.IntToTextMessage(m_Recovery.getRecoverCredit());
    level.IntToTextMessage(m_Recovery.getRecoverLevel());
    m_cardLogic.setTempLevel(m_Recovery.getRecoverLevel());
    Timer::g_Timer->StartTimer(m_Recovery.getRecoverLevel() + 3);

Answer 1

Personally I would recommend storing the game state in text format rather than binary. Binary data like this is non-portable, sometimes even between different versions of the same compiler on the same computer or even using different compiler configuration options.

That being said if you are going the binary route (or not) the main problem I see with the code is lack of error checking. And the whole idea of getting a Recovery object to hoist itself by its own petard make error checking very difficult.

I have knocked up something I think is more robust. I don't know the proper program structure you are using so this probably won't match what you need. But it may serve as an example of how this can be approached.

Most importantly always check for errors, report them where appropriate and return them to the caller.

enum gameStates
{
    MENU, STARTGAME, GAMEOVER, RECOVERY, RULES_OF_GAMES, VIEW_CARDS, STATISTIC
};

const std::string RECOVER_FILE = "files/recover.dat";

struct Recovery
{
    int m_RecoverLevel;
    int m_RecoverCredit;
    gameStates m_RecoverState;
};

struct WhateverClass
{
    Recovery m_Recovery;
    bool LoadRecoverFile(Recovery& rec);

public:
    bool recover();
};

// Supply the Recover object to be restored and
// return true or false to know it succeeded or not
bool WhateverClass::LoadRecoverFile(Recovery& rec)
{
    std::ifstream file(RECOVER_FILE, std::ios::binary);

    if(!file.is_open())
    {
        log("ERROR: opening the recovery file: " << RECOVER_FILE);
        return false;
    }

    if(!file.read(reinterpret_cast<char*>(&rec), sizeof(Recovery)))
    {
        log("ERROR: reading from recovery file: " << RECOVER_FILE);
        return false;
    }

    return true;
}

bool WhateverClass::recover()
{
    if(!LoadRecoverFile(m_Recovery))
        return false;

    credit.IntToTextMessage(m_Recovery.getRecoverCredit());
    level.IntToTextMessage(m_Recovery.getRecoverLevel());
    m_cardLogic.setTempLevel(m_Recovery.getRecoverLevel());
    Timer::g_Timer->StartTimer(m_Recovery.getRecoverLevel() + 3);

    return true;
}

Hope this helps.

Answer 2

Hi everyone Actually class StateManager content integers:

#ifndef STATEMANAGER_H_
#define STATEMANAGER_H_
enum gameStates {

    MENU, STARTGAME, GAMEOVER, RECOVERY, RULES_OF_GAMES, VIEW_CARDS, STATISTIC

};

class StateManager {
public:
    static StateManager* stateMachine;
    StateManager();
    virtual ~StateManager();
    gameStates getCurrentGameStates() const;
    void setCurrentGameStates(gameStates currentGameStates);
private:
    gameStates m_currentGameStates;
};

#endif /* STATEMANAGER_H_ */

Answer 3

It really depends on what a Recovery object contains. If it contains pointers to data, open resource descriptors and things like that you will not be able to store those on a file in a meaningful way. Restoring a pointer in this way may set its value, but the value it pointed will most certainly not be where you expect it to be anymore.

If Recovery is a POD this should work.

You may want to look at this question and this other question, which are similar to yours.

---

As Galik correctly points out, using

while (!File.eof()) {

doesn't make much sense. Instead, you should use

if ( File.read(/* etc etc */) ) {
    // Object restored successfully.
}
else {
   // Revert changes and signal that object was not loaded.
}

The caller of the function needs to have a way to know if the loading was successful. The method is already a member function, so a better definition could be:

/* Returns true if the file was read successfully, false otherwise.
 * If reading fails the previous state of the object is not modified.
 */
bool Recovery::LoadRecoverFile(const std::string & filename);

Answer 4

It probably is undefined behavior (unless Recovery is a POD made only of scalar fields).

It probably won't work if the Recovery class has a vtable, unless perhaps the process which is reading is the same process which wrote it. Vtables contain function pointers (usually, addresses of some machine code). And these function pointers would vary from one process to another one (even if they are running the same binary), e.g. because of ASLR.

It also won't work if Recovery contains other objects (e.g. a std::vector<std::shared_ptr<Recovery>> ...., or your gameStates), because these sub-objects won't be constructed correctly.

It could work sometimes. But what you apparently are looking for is serialization (then I would suggest using a textual format like JSON, but see also libs11n) or application checkpointing. You should design your application with those goals from the very start.