Access violation in a multithreaded application, C++

I am not very good in multithreading programming so I would like to ask for some help/advice.

In my application I have two threads trying to access a shared object. One can think about two tasks trying to call functions from within another object. For clarity I will show some parts of the program which may not be very relevant but hopefully can help to get my problem better.

Please take a look at the sample code below:

//DataLinkLayer.h 
class DataLinkLayer: public iDataLinkLayer {

public:
DataLinkLayer(void);
~DataLinkLayer(void);
};

Where iDataLinkLayer is an interface (abstract class without any implementation) containing pure virtual functions and a reference (pointer) declaration to the isntance of DataLinkLayer object (dataLinkLayer).

// DataLinkLayer.cpp
#include "DataLinkLayer.h"

DataLinkLayer开发者_运维知识库::DataLinkLayer(void) {

/* In reality task constructors takes bunch of other parameters  
 but they are not relevant (I believe) at this stage. */
dll_task_1* task1 = new dll_task_1(this); 
dll_task_2* task2 = new dll_task_2(this); 

/* Start multithreading */
task1->start(); // task1 extends thread class
task2->start(); // task2 also extends thread class
}

/* sample stub functions for testing */
void DataLinkLayer::from_task_1() {
printf("Test data Task 1");
}

void DataLinkLayer::from_task_2() {
printf("Test data Task 2");
}

Implementation of task 1 is below. The dataLinLayer interface (iDataLinkLayer) pointer is passed to the class cosntructor in order to be able to access necessary functions from within the dataLinkLayer isntance.

//data_task_1.cpp
#include "iDataLinkLayer.h"  // interface to DataLinkLayer
#include "data_task_1.h"

dll_task_1::dll_task_1(iDataLinkLayer* pDataLinkLayer) {
this->dataLinkLayer = pDataLinkLayer; // dataLinkLayer declared in dll_task_1.h
}

// Run method - executes the thread
void dll_task_1::run() {
// program reaches this point and prints the stuff
this->datalinkLayer->from_task_1();
}
// more stuff following - not relevant to the problem
...

And task 2 looks simialrly:

//data_task_2.cpp
#include "iDataLinkLayer.h"  // interface to DataLinkLayer
#include "data_task_2.h"

dll_task_2::dll_task_2(iDataLinkLayer* pDataLinkLayer){
this->dataLinkLayer = pDataLinkLayer; // dataLinkLayer declared in dll_task_2.h
}

// // Run method - executes the thread
void dll_task_2::run() {
// ERROR: 'Access violation reading location 0xcdcdcdd9' is signalled at this point
this->datalinkLayer->from_task_2();
}
// more stuff following - not relevant to the problem
...

So as I understand correctly I access the shared pointer from two different threads (tasks) and it is not allowed. Frankly I thought that I will be able to access the object nevertheless however the results might be unexpected.

It seems that something goes terribly wrong at the point when dll_task_2 tries to call the function using pointer to the DataLinkLayer. dll_task_2 has lower priority hence it is started afterwards. I don't understand why i still cannot at least access the object... I can use the mutex to lock the variable but I thought that the primary reason for this is to protect the variable/object.

I am using Microsoft Visual C++ 2010 Express. I don't know much about multithreading so maybe you can suggest a better solution to this problem as well as explain the reason of the problem.

The address of the access violation is a very small positive offset from 0xcdcdcdcd

Wikipedia says:

CDCDCDCD Used by Microsoft's C++ debugging runtime library to mark uninitialised heap memory

Here is the relevant MSDN page.

The corresponding value after free is 0xdddddddd, so it's likely to be incomplete initialization rather than use-after-free.

EDIT: James asked how optimization could mess up virtual function calls. Basically, it's because the currently standardized C++ memory model makes no guarantees about threading. The C++ standard defines that virtual calls made from within a constructor will use the declaring type of the constructor currently being run, not the final dynamic type of the object. So this means that, from the perspective of the C++ sequential execution memory model, the virtual call mechanism (practically speaking, a v-table pointer) must be set up before the constructor starts running (I believe the specific point is after base subobject construction in the ctor-initializer-list and before member subobject construction).

Now, two things can happen to make the observable behavior different in a threaded scenario:

First, the compiler is free to perform any optimization that would, in the C++ sequential execution model, act as-if the rules were being followed. For example, if the compiler can prove that no virtual calls are made inside the constructor, it could wait and set the v-table pointer at the end of the constructor body instead of the beginning. If the constructor doesn't give out the this pointer, since the caller of the constructor also hasn't received its copy of the pointer yet, then none of the functions called by the constructor can call back (virtually or statically) to the object under construction. But the constructor DOES give away the this pointer.

We have to look closer. If the function to which the this pointer is given is visible to the compiler (i.e. included in the current compilation unit), the the compiler can include its behavior in the analysis. We weren't given that function in this question (the constructor and member functions of class task), but it seems likely that the only thing that happens is that said pointer is stored in a subobject which is also not reachable from outside the constructor.

"Foul!", you cry, "I passed the address of that task subobject to a library CreateThread function, therefore it is reachable and through it, the main object is reachable." Ah, but you do not comprehend the mysteries of the "strict aliasing rules". That library function does not accept a parameter of type task *, now does it? And being a parameter whose type is perhaps intptr_t, but definitely neither task * nor char *, the compiler is permitted to assume, for purposes of as-if optimization, that it does not point to a task object (even if it clearly does). And if it does not point to a task object, and the only place our this pointer got stored is in a task member subobject, then it cannot be used to make virtual calls to this, so the compiler may legitimately delay setting up the virtual call mechanism.

But that's not all. Even if the compiler does set up the virtual call mechanism on schedule, the CPU memory model only guarantees that the change is visible to the current CPU core. Writes may become visible to other CPU cores in a completely different order. Now, the library create thread function ought to introduce a memory barrier that constrains CPU write reordering, but that fact that Koz's answer introducing a critical section (which definitely includes a memory barrier) changes the behavior suggests that perhaps no memory barrier was present in the original code.

And, CPU write reordering can not only delay the v-table pointer, but the storage of the this pointer into the task subobject.

I hope you have enjoyed this guided tour of one small corner of the cave of "multithreaded programming is hard".

printf is not, afaik, thread safe. Try surrounding the printf with a critical section.

To do this you InitializeCriticalSection inside iDataLinkLayer class. Then around the printfs you need an EnterCriticalSection and a LeaveCriticalSection. This will prevent both functions entering the printf simultaneously.

Edit: Try changing this code:

dll_task_1* task1 = new task(this); 
dll_task_2* task2 = new task(this);

to

dll_task_1* task1 = new dll_task_1(this); 
dll_task_2* task2 = new dll_task_2(this);

Im guessing that task is in fact the base class of dll_task_1 and dll_task_2 ... so, more than anything, im surprised it compiles ....

I think it's not always safe to use 'this' (i.e. to call a member function) before the end of the constructor. It could be that task are calling member function of DataLinkLayer before the end of DataLinkLayer constructor. Especially if this member function is virtual: http://www.parashift.com/c++-faq-lite/ctors.html#faq-10.7

I wanted to comment on the creation of the DataLinkLayer.

When I call the DataLinkLayer constructor from main:

int main () {
DataLinkLayer* dataLinkLayer = new DataLinkLayer();
while(true); // to keep the main thread running
}

I, of coruse, do not destruct the object, this is first. Now, inside the DataLinkLayer cosntructor I initialize many (not only these two tasks) other objects isntances and pass to most of them dataLinkLayer pointer (using this). This is legal, as far as I am concerned. Put it further - it compiles and runs as expected.

What I became curious about is the overall design idea that I am following (if any :) ).

The DataLinkLayer is a parent class that is accessed by several tasks which try to modify it parameters or perform some other processing. Since I want that everything remain as decoupled as possible I provide only interfaces for the accessors and encapsulate the data so that I don't have any global variables, friend functions etc.

It would have been a pretty easy task to do if only multithreading would not be there. I beleive I will encounter many other pitfalls on my way.

Feel free to discuss it please and merci for your generous comments!

UPD:

Speaking of passing the iDataLinkLayer interface pointer to the tasks - is this a good way to do it? In Java it would be pretty usual thing to realize a containment or so called strategy pattern to make things decoupled and stuff. However I am not 100% sure whether it is a good solution in c++... Any suggestions/commnets on it?