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);

`while (!File.eof())` is the wrong way to loop and I am not sure why they are even looping here? Also what are they going to do if only half the data is read? There is no way to signal that the object did not load properly. If you are going to store objects like this, I would rather an external manager controlled the process where errors could be managed more easily. — Galik, Oct 24 '14 at 14:53

Basile Starynkevitch · Answer 1 · 2014-10-24T14:54:23.017

2

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.

edited Oct 24 '14 at 14:54

answered Oct 24 '14 at 14:43

Basile Starynkevitch

223,805
18
296
547

Just as a note: It actually works on MinGW 4.8.2, Windows 8.1. – Zingam Oct 24 '14 at 14:45
1

Why is it undefined behaviour? `A pointer to an object can be explicitly converted to a pointer to a different object type.` from the standard about `reinterpret_cast` (http://stackoverflow.com/questions/4748232/reinterpret-cast) – Svalorzen Oct 24 '14 at 14:50
1

@Zingam: explain what you mean by "actually works". On different processes? On slightly different executables (e.g. you added or improved some routine unrelated to `Recovery`?) – Basile Starynkevitch Oct 24 '14 at 14:55
I am not questioning your answer. What I mean is that the program does not crash and does recover the data as expected by the original author. I am trying to understand the implications of doing something like that and to explain them. I personally would use a textual file just as you suggested. – Zingam Oct 24 '14 at 15:04
2

@Zingam Undefined programs are allowed to not crash and even occasionally give the impression of actually working. That doesn't make them less borken. – molbdnilo Oct 24 '14 at 15:08

score 2 · Answer 2 · edited May 23 '17 at 11:57

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);

@Zingam This function has a `vtable`, since you declared a `virtual` destructor. So no, it won't work. If you remove the virtual desctructor, you'd still need to show what's inside `gameState`. — Svalorzen, Oct 24 '14 at 15:01
@Zingam Note that the fact that it is currently working does not give you any kind of guarantee. — Svalorzen, Oct 24 '14 at 15:04

score 1 · Accepted Answer · answered Oct 24 '14 at 18:45

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.

I like this as a solution to this problem. Just as a side note: The cited code is not mine. I wouldn't do that ever. I am trying to explain what's wrong with the code to my friend. — Zingam, Oct 24 '14 at 19:52

score -1 · Answer 4 · answered Oct 24 '14 at 16:06

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_ */

C++ object that modifies itself in memory

4 Answers4