Error deleting std::vector in a DLL using the PIMPL idiom

Question

I have the following code:

In DLL1:

in .h file:

class MyClass
{
public:
    MyClass();
private:
    std::string m_name;
};

class __declspec(dllexport) Foo
{
private:
    struct Impl;
    Impl *pimpl;
public:
    Foo();
    virtual ~Foo();
};

struct Foo::Impl
{
    std::vector<MyClass> m_vec;
    std::vector<MyClass> &GetVector() { return m_vec; };
};

in .cpp file:

Foo::Foo() : pimpl ( new Impl )
{
}

Foo::~Foo()
{
    delete pimpl;
    pimpl = NULL;
}

[EDIT]

In DLL2

in .h

class Bar : public Foo
{
public:
    Bar();
    virtual ~Bar();
};

in .cpp:

Bar::Bar()
{
}

Bar::~Bar()
{
}

In DLL3:

extern "C" __declspec(dllexport) Foo *MyFunc(Foo *param)
{
    if( !param )
        param = new Bar();
    return param;
}

In main application:

void Abc::my_func()
{
    Foo *var = NULL;
// loading the DLL3 and getting the address of the function MyFunc
    var = func( var );
    delete var;
}

Now, I presume that the copy constructor should be private as it does not make sense to copy both Foo and Bar objects.

Now the question I have is: should Bar also have copy constructor and assignment operator? [/EDIT]

Notice that MyClass is not exported and does not have a destructor.

Is this generally how you write the code?

The problem is that I have a crash on Windows (8.1 + MSVC 2010, if it matters). I can post more code if needed, but for now just want to make sure I don't do something obviously wrong.

The crash happens after I step out of the Base destructor and the stack trace says:

ntdll.dll!770873a6() [Frames below may be incorrect and/or missing, no symbols loaded for ntdll.dll] ntdll.dll!7704164f()
ntdll.dll!77010f01() KernelBase.dll!754a2844()
dll1.dll!_CrtIsValidHeapPointer(const void * pUserData) Line 2036 C++ dll1.dll!_free_dbg_nolock(void * pUserData, int nBlockUse) Line 1322 + 0x9 bytes C++ dll1.dll!_free_dbg(void * pUserData, int nBlockUse) Line 1265 + 0xd bytes C++ dll1.dll!operator delete(void * pUserData) Line 54 + 0x10 bytes C++ dll1.dll!Foo::`vector deleting destructor'() + 0x65 bytes C++

Thank you.

UPDATE:

Even if I put following code in the

extern "C" __declspec(dllexport) Foo *MyFunc(Foo *param)
{
    param = new Bar();
    delete param;
    return param;
}

The program is still crashing in the delete param operation in the same place.

It looks like the destructor of the std::vector is called later, after the destructor of Foo is called. Is it how it suppose to be?

UPDATE2:

After carefully running this under debugger, I see that the crash happens inside "void operator delete(void *pUserData);". The pUserData pointer has an address of "param".

The DLL1 is built with this:

C++

/ZI /nologo /W4 /WX- /Od /Oy- /D "WIN32" /D "_DEBUG" /D "_LIB" /D "_UNICODE" /D "UNICODE" /Gm /EHsc /RTC1 /MTd /GS /fp:precise /Zc:wchar_t /Zc:forScope /Fp"Debug\dll1.pch" /Fa"Debug\" /Fo"Debug\" /Fd"Debug\vc100.pdb" /Gd /analyze- /errorReport:queue

Librarian /OUT:"C:\Users\Igor\OneDrive\Documents\dbhandler1\docview\Debug\dll1.lib" /NOLOGO

The DLL2 was built with:

C++

/I"..\dll1\" /Zi /nologo /W4 /WX- /Od /Oy- /D "WIN32" /D "_USRDLL" /D "DLL_EXPORTS" /D "_DEBUG" /D "_CRT_SECURE_NO_DEPRECATE=1" /D "_CRT_NON_CONFORMING_SWPRINTFS=1" /D "_SCL_SECURE_NO_WARNINGS=1" /D "_UNICODE" /D "MY_DLL_BUILDING" /D "_WINDLL" /D "UNICODE" /Gm- /EHsc /RTC1 /MTd /GS /fp:precise /Zc:wchar_t /Zc:forScope /GR /Fp"vc_mswud\dll2\dll2.pch" /Fa"vc_mswud\dll2\" /Fo"vc_mswud\dll2\" /Fd"vc_mswud\dll2.pdb" /Gd /analyze- /errorReport:queue 

Linker

/OUT:"..\myapp\vc_mswud\dll2.dll" /INCREMENTAL /NOLOGO /LIBPATH:"..\docview\Debug\" /DLL "dll1.lib" "kernel32.lib" "user32.lib" "gdi32.lib" "comdlg32.lib" "winspool.lib" "winmm.lib" "shell32.lib" "shlwapi.lib" "comctl32.lib" "ole32.lib" "oleaut32.lib" "uuid.lib" "rpcrt4.lib" "advapi32.lib" "version.lib" "wsock32.lib" "wininet.lib" /MANIFEST /ManifestFile:"vc_mswud\dll2\dll2.dll.intermediate.manifest" /ALLOWISOLATION /MANIFESTUAC:"level='asInvoker' uiAccess='false'" /DEBUG /PDB:"vc_mswud\dll2.pdb" /PGD:"C:\Users\Igor\OneDrive\Documents\myapp\dll2\vc_mswud\dll2.pgd" /TLBID:1 /DYNAMICBASE /NXCOMPAT /IMPLIB:"vc_mswud\dll2.lib" /MACHINE:X86 /ERRORREPORT:QUEUE 

Does anybody see any issues with the way my libraries are built?

Answer 1

Without further code available for analysis, an important bug I see in your posted code is that your Foo class is a resource manager violating the so called Rule of Three.

Basically, you dynamically allocate an Impl instance in the Foo constructor using new, you have a virtual destructor for Foo releasing the managed resource (pimpl) with delete, but your Foo class is vulnerable to copies.
In fact, the compiler generated copy constructor and copy assignment operators perform member-wise copies, which are basically shallow-copies of the pimpl pointer data member: this is a source of "leaktrocities".

You may want to declare private copy constructor and copy assignment for Foo, to disable compiler-generated member-wise copy operations:

// Inside your Foo class definition (in the .h file):
...

// Ban copy
private:
    Foo(const Foo&); // = delete
    Foo& operator=(const Foo&); // = delete

Note: The C++11's =delete syntax to disable copies is not available in MSVC 2010, so I embedded it in comments.

---

Not directly related to your problem, but maybe worth noting:

1. In your Foo::Impl structure, since the m_vec data member is already public, I see no immediate reason to provide an accessor member function like GetVector().

2. Starting with C++11, consider using nullptr instead of NULL in your code.

Answer 2

The problem is that you've allocated Bar in DLL3, which includes the contained instance of Foo. However, you deleted it in the main application via the Foo*, which has done the deletion in DLL1 (as seen in your stack trace).

The debug heap checker has caught you allocating memory in one module and freeing it in another module.

---

Detailed explanation of the issue:

Calling new Foo(args...) does roughly the following:

pFoo = reinterpret_cast<Foo*>(::operator new(sizeof(Foo)));
pFoo->Foo(args...);
return pFoo;

In the MS Visual Studio C++ object model, this is inlined at the call of new Foo, so happens where you call the new statement.

Calling delete pFoo does roughly the following:

pFoo->~Foo();
::operator delete(pFoo);

In the MS Visual Studio C++ object model, both of these operations are compiled into ~Foo, in the Foo::`vector deleting destructor'(), which you can see in psuedocode at Mismatching scalar and vector new and delete.

So unless you change this behaviour, ::operator new will be called at the site of new Foo, and ::operator delete will be called at the site of the closing brace of ~Foo.

I haven't detailed virtual or vector behaviours here, but they don't carry any further surprises beyond the above.

Class-specific overloads of operator new and operator delete are used instead of ::operator new and ::operator delete in the above, if they exist, which lets you control where ::operator new and ::operator delete are called, or even to call something else entirely (e.g. a pool allocator). That's how you explicitly solve this issue.

I understood from MS Support Article 122675 that MSVC++ 5 and later is supposed to not include the ::operator delete call in the destructor of dllexport/dllimport classes with a virtual destructor, but I never managed to trigger that behaviour, and have found it much more reliable to be explicit about where my memory is allocated/deallocated for DLL-exported classes.

---

To fix this, give Foo class-specific overloads of operator new and operator delete, e.g.,

class __declspec(dllexport) Foo
{
private:
    struct Impl;
    Impl *pimpl;
public:
    static void* operator new(std::size_t sz);
    static void operator delete(void* ptr, std::size_t sz)
    Foo();
    virtual ~Foo();
};

Don't put the implementations in the header, or it'll be inlined, which defeats the point of the exercise.

void* Foo::operator new(std::size_t sz)
{
    return ::operator new(sz);
}

void Foo::operator delete(void* ptr, std::size_t sz)
{
    return ::operator delete(ptr);
}

Doing this only for Foo will cause both Foo and Bar to be allocated and destroyed in the context of DLL1.

If you'd rather Bar be allocated and deleted in the context of DLL2, then you can give it one as well. The virtual destructor will ensure that the right operator delete will be called even if you delete the base pointer as in your given example. You might have to dllexport Bar though, as the inliner can sometimes surprise you here.

See MS Support Article 122675 for some more details, although you've actually bounced off the opposite problem than the one they describe there.

---

Another option: make Foo::Foo protected, and Bar::Bar private, and expose static factory functions for them from your DLL interface. Then the ::operator new call is in the factory function rather than the caller's code, which will put it in the same DLL as the ::operator delete call, and you get the same effect as providing class-specific operator new and operator delete, as well as all the other advantages and disadvantages of factory functions (which are a great improvement once you stop passing raw pointers around and start using unique_ptr or shared_ptr depending on your requirements).

To do this, you have to trust the code in Bar to not call new Foo, or you've brought the problem back. So this one is more protection by convention, while class-specific operator new/operator delete expresses the requirement that memory allocation for that type be done in a certain way.