Critical sections and thread safety

[AcarX]

I'm trying to create a server application using IOCP and I have a bottleneck with thread safety. Take these 3 functions for example:

void func1(Client * client){
	if(client == NULL){
		// Print error
		return;
	}
	EnterCriticalSection(&client->CS);

	// Use some class functions/variables

	LeaveCriticalSection(&client->CS);
}

void func2(SOCKET_OBJ * sock){
	EnterCriticalSection(&sock->client->CS);
	LeaveCriticalSection(&sock->client->CS);
	delete sock->client;
}

This looks very error prone to me. For example:
Thread1 acquires critical section within func1.
Thread2 and Thread3 enters queue within func1 and func2.
Thread3 acquires critical section and deletes client which resulting as critical section being destroyed. Thread2 now will throw access violation exception I believe?

Either way this is just one of the scenarios I could think of. There're probably many more like this. How would I go about solving this?

[Zhuge]

I would not have the critical section object you are locking be accessible only through the pointer it is protecting.

As you've noticed, you can't properly protect client that way:

Even just looking at func2(), you are deleting sock->client without even protecting it in a critical section! That means any other thread could be messing with client when you do that. Similarly, on line 2 you check client but between that and entering the critical section, client could be nulled out or deleted by another thread.

The problem is that you have created a catch-22 situation; you need to enter the critical section to access client, but cannot access client without entering the critical section.

The solution here, I think, is to simply store the critical section outside of where the client pointer is so it can be separately entered/exited.

[AcarX]

Thanks for the reply. Would storing the critical section object inside SOCKET_OBJ structure solve the problem? Perhaps something like:

void func1(SOCKET_OBJ * sock){
	EnterCriticalSection(&sock->CS);
	
	Client * client = sock->client;
	if(client == NULL){
		// Print error
		return;
	}

	// Use some class functions/variables

	LeaveCriticalSection(&sock->CS);
}

void func2(SOCKET_OBJ * sock){
	EnterCriticalSection(&sock->CS);
	delete sock->client;
	LeaveCriticalSection(&sock->CS);
}

I can't think of any bad scenario with this approach but if there's one please let me know.

[Zhuge]

That should avoid the issues from before. Just ensure the lifetime of sock is longer than any threads operating on it, as you wouldn't want it to become null or be deleted while threads that relied on it were still alive.

[AcarX]

Will do. Thanks for the help.