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?

Perse

Well I hope you never make a copy of Foo because the compiler-generated copy constructor will perform a shallow copy of pimpl, leading to a double-delete. – Stheno

You are not following the Rule of Three. Now, with that aside, I think you should show how you are using Foo. It's not clear whether you are following guidelines for memory allocation and freeing with DLLs. Specifically, whoever allocated Foo should free Foo. Is this the case? – Lashelllasher

This isn't the problem, but there's no point in setting pimpl to NULL in Foo's destructor. The object is going away, so pimpl goes away, too. – Passivism

You might need to post more code. There's no Base in the code you posted, so failures stepping out of its destructor are very hard to help with. – Crucial

I created small project using the posted code and didn't get crash. Means there is something else and probably not in the code that you've shared with us. btw, I recommend you the same (make small project based on your post). May be it'll help you identify the issue faster. – Ellissa

@StahlRat, yes, I might do just that. I thought that maybe there is something obvious I'm missing... – Perse

@TBBle, I will post more code when off of work. Hopefully someone here will stick around. – Perse

@PeteBecker, I was trying to see if its a double free and so to prevent that set it to NULL. – Perse

@paddy, yes, that is the case. I have a test code that allocates pointer and then deletes it immediately. – Perse

@AndyG, no it is being passed by the pointer/reference. – Perse

@Igor: "I was trying to see if its a double free and so to prevent that set it to NULL.". That wouldn't prevent it. You'd be setting NULL on a copy. Try to disable (=delete) or implement the copy constructor and see what happens. – Stheno

@TBBle, I posted some additional code. – Perse

@AndyG, more code added with the context. – Perse

@Igor: Bar suffers from the same shallow copy problem as Foo – Stheno

"The crash happens after I step out of the Base destructor" Do you mean "Bar destructor" here? There's still no class Base in the posted code. – Crucial

From the update: dll1.dll!Foo::`vector deleting destructor' is not the std::vector destructor. It's the 'final clean-up code' of Foo, so the code that runs right after the written code of ~Foo completes. This is an internal detail of the Visual C++ object model. – Crucial

By-the-way, I don't believe this is pImpl-related. You should find if you replaced Impl* pImpl with a simple bool, the same behaviour would happen. – Crucial

There is no longer a clear question here. The title says you're getting and error during the delete, but the question (before all the edits) is about whether you should implement a copy constructor. There are a lot of red herrings and unrelated mistakes distracting from the issues. This question is too unfocused to help future visitors. I suggest starting a new, very specific question. – Toscano

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.

Seminal

thx. I will try it when I get back home. Will post here if anything. Also, do they have to be private? – Perse

@Igor: Not sure I understand your question. Anyway, as already written, declaring private copy constructor and assignment operator disables invoking automatically-compiler-generated bogus shallow-copies. – Seminal

I added copy constructor stub and operator= stub returning *this as private members and its still crashing. Maybe they ought to be public with real implementation? – Perse

please see the update. Calling new and then delete in the same function still causing it to crash. – Perse

You should provide a simple compilable repro for the crash, to make it a proper StackOverflow question. As it is now, it seems to me more like a request for consulting, which I'm unable to do without proper budget. Others with more available bandwidth than me might be able to help. Just a note: in your last question update, you have a MyFunc body with ...delete param; return param;. This is a bug farm, since you return a dangling (deleted) pointer. You may need someone able to tutor you to some basic C++ and DLL issues. Moreover, the bug might be in code you hadn't showed. Good luck. – Seminal

Yes, I can do that. Which file sharing service I can upload the project? Because it may as well be some project settings... I am compiling and running it under Windows 8.1 + MSVC 2010. The code in update is to reference that even if I delete the pointer right after allocating memory in the same function it still crashes. And yes the pointer is dangling, but to be absolutely sure I ran it thru the debugger. – Perse

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.

Crucial

please see the update. Even if I allocate memory and then call delete on the pointer it still crashes. – Perse

If the stack trace hasn't changed, then you're still allocating in DLL3, and deleting in DLL1. The memory is being allocated where new is called, and deleted where ~Foo is defined. It doesn't matter where you move the delete. – Crucial

why there is an extra "size" parameter in the delete() function? And IIUC, with that addition everything memory management will happen inside DLL1/DLL2, right? – Perse

thank you. After adding constructor/destructor to Foo it worked and didn't crash. – Perse

I spoke too fast, it worked when I put new and delete in one function. But after splitting it crashed again. It looks like the pointer is bad. I guess I should just set the Foo class inside the main application and try it this way. – Perse

Moreover it looks like the pointer is good until the library is loaded. When the library is unloaded, the pointer becomes bad. – Perse

That's right. If you use the class-specific operator new and operator delete, then the memory is allocated inside the DLL where they are implemented. And so if you unload the DLL, then that memory is now invalid and your program will die. On the other hand, if your memory was allocated in the main app, and you unload the DLL, then then pointer is valid, but all the method calls will be to invalid pointers, and your program will die. So don't unload the DLL while you still have objects that were created by the DLL. – Crucial

Class-specific operator delete can take a second size_t parameter, which is the same value passed to the class-specific operator new in the first place. However, it's not needed if you're passing straight through to ::operator delete so you could leave the parameter off, as in the examples on the linked pages. It's useful if you're doing something like a sized-based pool implementation, and don't want to track which pool a pointer came from, or search every pool for a pointer. – Crucial