0

I have a strange 'destructor' Behavior in C++

Here is the code I call :

_log->OnCommit(); delete _log;

The problem is that when I call "delete _log;" it crash because the variable 'Entries' is invalid !!!!

Do you know why ?

Here is my class code :

struct TransactionLogEntry { DependencyObject* Object; bool IsAttached; bool IsDeleted; bool IsUpdated; }; class TransactionLog { public: TransactionLog(); ~TransactionLog(); void OnCommit(); map<DependencyObject*, TransactionLogEntry*> Entries; }; void TransactionLog::OnCommit() { map<DependencyObject*, TransactionLogEntry*>::iterator it; for(it = Entries.begin(); it != Entries.end(); it++) { TransactionLogEntry* entry = (TransactionLogEntry*)(*it).second; if (entry->IsDeleted) delete entry->Object; delete entry; } Entries.clear(); } TransactionLog::~TransactionLog() { map<DependencyObject*, TransactionLogEntry*>::iterator it; for(it = Entries.begin(); it != Entries.end(); it++) { TransactionLogEntry* entry = (TransactionLogEntry*)(*it).second; delete entry; } Entries.clear(); }
Improve this question
7
  • 2
    Why are you casting it->second? Commented Feb 3, 2011 at 13:04
  • Does onCommit returns correctly? have you checked it under a debugger? Commented Feb 3, 2011 at 13:04
  • 2
    TransactionLogEntry's dtor should probably be handling delete entry->Object. Commented Feb 3, 2011 at 13:05
  • 2
    post a complete, compilable but short example of the problem. The bug might be somewhere else in code you didn't show. Tips: Get rid of the C-style casts. We have a shortcut for (*it).second and it's called it->second. Commented Feb 3, 2011 at 13:07
  • 2
    Start off by isolating your problem code. I.e. simplify by taking out what you have there and also TransactionLogEntry and run that in separation. Reproduce the problem. Now you have less moving part to look at. If you already have unit tests, this is the place to start. Commented Feb 3, 2011 at 13:25

5 Answers 5

Reset to default
3

It's hard to see without complete code, however I can notice that you're violating the rule of the big three (you have a destructor, but no copy constructor or assignment operator) and this means looking for troubles.

My wild guess is that you're copy-constructing log or other similar problems and once you enter in UB mode then anything can happen, including raising errors in places that should be ok.

Improve this answer
Sign up to request clarification or add additional context in comments.

6 Comments

Nice catch. I know to look for this and I still didn't catch it before seeing your answer. Automatically generated copy ctors and assignment op are a bane. :(
Thanks, Yes I do 'delete entry' but later I 'clear' the map, so why should I do something like this ? delete entry; Entries[it->first] = 0; I have add this line and it still crash :-( Thanks
@user346113: Because any copies of the TransactionLog object would copy the map and thus copy the pointers. Then you clear one map after deleting the pointer, but not any copies of that map. This leads to UB when you later access entries pointed to by the copied map.
Also, TransactionLog are only used as pointer. I think that there is no "copy" constructor related problem ! At least... I think !
user346113: If you don't think there is a problem. Make the copy constructor and assignment operator private (and undefined) and see if it compiles. If it does then you should be OK it it fails to compile then you have a problem.
|
1

Are you storing naked pointers in the Entries map? If so, you should investigate using boost::shared_ptr (or tr1::shared_ptr if youve got that) instead. This greatly simplifies storage management (for example you can delete the for loop in TransactionLog::OnCommit(), and just call Entries.clear().

Improve this answer

Comments

1

As said, you're missing a copy ctor and assignment operator for TransactionLog. Here is the problem, simplified:

struct A { int *p; A() : p (new int()) {} ~A() { delete p; } }

This properly allocates and destroys an object, right? Not quite:

void example() { A a; A b = a; // Missing copy ctor; could also happen when passing/returning by value. b = a; // Same result through missing assignment operator. assert(a.p == b.p); // Here is the problem! // When b is destroyed, it deletes the pointer. // When a is destroyed, it attempts to delete the deallocated pointer, // leading to undefined behavior. }

And here is the same problem written more closely to your code:

struct A { map<string, int*> m; A() { m["abc"] = new int(); } ~A() { for (map<string, int*>::iterator x = m.begin(); x != m.end(); ++x) { delete x->second; } } void on_commit() { for (map<string, int*>::iterator x = m.begin(); x != m.end(); ++x) { delete x->second; } m.clear(); } }

The solution is to declare a copy ctor and assignment operator for your class. Even if your class is "non-copyable", you should still declare them, but make them private and don't define them:

struct A { int *p; A() : p (new int()) {} ~A() { delete p; } private: A(A const&); A& operator=(A const&); }

When they are private, any use (in an inaccessible context) will be a compiler error.

Improve this answer

4 Comments

Thanks Fred, I have understand the explanation... but there is NO copy anywhere ! I just do log->commit(); delete log; The commit works fine. But after the 'Entries' is invalid. Also 'log' is always a pointer, it is not like in your example where a,b are values ! So, when in 'commit' I remove 'Entries.clear' and 'delete entry' I have no crash ! So, there is a problem with theses lines !
@user346113: Then you need to give us a complete and reproducible test case. This answer and the other point out a very significant problem in your code; you should still make the copy ctor and op= private if you don't want to use them.
Of course, it is good idea that you have. I have try it (In case I have forgot something). But none is called and the problem is still there ! It just prove that the problem is somewhere else !
@user346113: That's why we need a complete and reproducible test case.
0

Looking at what you have in the OnCommit function:

... if (entry->IsDeleted) delete entry->Object; delete entry; ...

it looks like you check if something is deleted. If so, you delete something inside it and always delete the object again. Looks like you're asking for problems.

Improve this answer

Comments

0

here may 2 cents:

  1. as it was said before, try to avoid using naked c/c++ pointers to objects, instead use some kind of smart ptr (e.g. boost::shared_ptr) or auto_ptr for simple holding/releasing resources (but not in stl containers due to auto_ptr specific)

  2. about this crash: there is nothing to prevent you from filling map object with different keys but the same values. so it becomes possible to delete object twice or more times which definitely leads to crash (you can delete pointer twice or more if it is equal to 0). so write this:

    delete ptr; ptr = 0;

instead of just deleting a pointer

Improve this answer

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.