2

This post will be a bit large, so sorry in advance. Anyway, I'm getting an exception when running my program in debug mode(Visual Studio 2010) that I can't quite figure why happens:

Unhandled exception at 0x5524ad4a (msvcp100d.dll) in CppTest1.exe: 0xC0000005: Access violation reading location 0xfeeefef2.

I'm quite new to C++ and I'm trying to implement a pimpl-like pattern class structure like the following:

declaration.h

#include <vector>
class A
{
public:
  struct aStruct
  {
    std::string aString;
    std::vector<std::string> moreStrings;
  };
  A();
  ~A();
  void addSomething(aStruct thing);
private:
  class Implementation;
  Implementation* instance;
};

class B
{
public:  
  B();
  ~B();
  void doWork();
private:
  class Implementation;
  Implementation* instance;
};

class A::Implementation
{
public:
  Implementation();
  ~Implementation();
  void addSomething(aStruct thing);
private:
  std::vector<A::aStruct> bunchOfStructs;
};

class B::Implementation
{
public:
  Implementation();
  ~Implementation();
  void doWork();
private:
  A member;
};

main.cpp

#include "declaration.h"
A::A() : instance(new Implementation) {}
A::~A() { delete instance; }
void A::addSomething(aStruct thing) { instance->addSomething(thing);}

A::Implementation::Implementation() {}
A::Implementation::~Implementation() {}
void A::Implementation::addSomething(aStruct thing) { bunchOfStructs.push_back(thing);}

B::B() : instance(new Implementation) {}
B::~B() {delete instance;}
B::Implementation::Implementation() { doWork();}
B::Implementation::~Implementation() {}
void B::Implementation::doWork()
{
  A a; 
  member = a;
}

int main( int argc, char* argv[] ) 
{
  B b;  
  return 0;
}

Now when the code escapes the main block the destructor for B is called, which in turn deletes the Implementation which in turn calls the destructor for the instance of Implementation it holds and so on until the destructor for the vector bunchOfStructs held in the instance of A is called. And this is where it fails.

The call stack looks like this:

msvcp100d.dll!std::_Container_base12::_Orphan_all()  Line 201 + 0x12 bytes  C++
CppTest1.exe!std::vector<A::aStruct,std::allocator<A::aStruct> >::_Tidy()  Line 1304 + 0xb bytes    C++
CppTest1.exe!std::vector<A::aStruct,std::allocator<A::aStruct> >::~vector<A::aStruct,std::allocator<A::aStruct> >()  Line 706   C++
CppTest1.exe!A::Implementation::~Implementation()  Line 8 + 0x2b bytes  C++
CppTest1.exe!A::Implementation::`scalar deleting destructor'()  + 0x2b bytes    C++
CppTest1.exe!A::~A()  Line 4 + 0x50 bytes   C++
CppTest1.exe!B::Implementation::~Implementation()  Line 14 + 0x2b bytes C++
CppTest1.exe!B::Implementation::`scalar deleting destructor'()  + 0x2b bytes    C++
CppTest1.exe!B::~B()  Line 12 + 0x50 bytes  C++
CppTest1.exe!main(int argc, char * * argv)  Line 24 + 0x12 bytes    C++

As far as I understand 0xfeeefeee is a fill pattern used in Visual Studio during debug. And the exception message indicates that I'm trying to access something that has been deleted already? But I don't quite understand why it happens. Something is apperantly being destroyed before I think it is. If it wasn't for the assignment member = a the code will run. Also if I implemented this without the pattern it would seemingly run fine. E.g:

thisworks.h

#include <vector>
class C
{
public:
  struct aStruct
  {
    std::string aString;
    std::vector<std::string> moreStrings;
  };
  C();
  ~C();
  void addSomething(aStruct thing);
private:
  std::vector<C::aStruct> bunchOfStructs;
};
class D
{
public:
  D();
  ~D();
  void doWork();
private:
  C member;
};

main.cpp

#include "thisworks.h"
C::C() {}
C::~C() {}
void C::addSomething(aStruct thing) { bunchOfStructs.push_back(thing); }

D::D() { doWork(); }
D::~D() {}
void D::doWork()
{
  C c;
  member = c;
}
int main( int argc, char* argv[] ) 
{
  D d;
  return 0;
}

Now I realize that there are probably better ways of doing this, however since I'm still trying to learn C++ and I've already invested some time trying to figure out why this is a problem in the first place I would really like to understand the problem.

Beepboop
  • 23
  • 1
  • 4
  • The purpose of using pimpl is to hide internal class detail from clients that include the header. But it looks like you're sticking the implementation details in the header too -- if you're going to do that why bother with pimpl at all? – greatwolf Sep 12 '13 at 09:02
  • Immediately note the fault is in `msvcp100d.dll`, which means it is an std-object likely being cleaned up twice. The byte pattern you see is fill for the RT when it has already freed the underlying memory. So something that is *holding* a std object is likely being double-deleted or accessed after-delete. – WhozCraig Sep 12 '13 at 09:03
  • 2
    You're violating the rule of three, which is probably the reason - as Enigma points out. http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three – Arne Kjetil Andersen Sep 12 '13 at 09:05
  • @greatwolf Sorry, I should probably have been more clear on that. I added them into the same header to save some space for this example as I felt the post was getting a bit bloated. – Beepboop Sep 12 '13 at 11:01

2 Answers2

3

I think that you copy the instance pointer within A when you perform member = a; In other words, member and a point to the same Implementation. When A gets deleted the instance gets deleted. While member still tries to access instance.

Enigma
  • 1,699
  • 10
  • 14
2

When you execute member = a; you are performing a bit-to-bit copy of the object, which is of type A. At this point member.instance is equal to a.instance, i.e. it is a pointer to the memory allocated in A::A() when creating a.

When B::Implementation::doWork() terminates, the destructor for a is called, which in turn deletes a.instance; this operation makes member.instance a dangling pointer.

Stefano Falasca
  • 8,837
  • 2
  • 18
  • 24