Accessing virtual member function causes STATUS_ACCESS_VIOLATION to be thrown

I have a class that looks like this:

Code:

/* pdConState is going to be a base class */
	class pdConState
	{
                
		public:
		pdConState() { }
		virtual void ProcessUserInput(pdPlayer* ch);
		
	};

ProcessUserInput() is defined in another file.

Code:

void pdConState::ProcessUserInput(pdPlayer* ch)
{
/* Do stuff */
}

When I call it from somewhere else like this:

Code:

	for (int i = 0; i < player.size(); i++)
	{
		pdPlayer* ch=&player.at(i);
		if(ch->GetCmdBuffer()=="")
			continue;
		ch->GetState()->ProcessUserInput(ch);
	}

The program throws a segmentation fault / STATUS_ACCESS_VIOLATION when it hits the line in bold.

Code:

Program received signal SIGSEGV, Segmentation fault.
0x004047ac in pdmud::pdmudServer::ProcessInput (this=0x28cc78) at pdmud.cxx:111
111                     ch->GetState()->ProcessUserInput(ch);
(gdb) n
     12 [main] pdmud 1764 exception::handle: Exception: STATUS_ACCESS_VIOLATION
   1321 [main] pdmud 1764 open_stackdumpfile: Dumping stack trace to pdmud.exe.stackdum

However, making the function a plain function instead of a virtual one stops the crashing. After doing some Googling, I'm guessing that it has something to do with virtual functions being dynamically bound instead of statically bound, but I still have no idea of how to fix the problem.

You are probably doing something that results in undefined behaviour, but with the given information I cannot tell what that might be. Post the smallest and simplest compilable program that demonstrates the problem.

My guess is that

Code:

ch->GetState()

returns an invalid pointer. It uses this pointer to find out the address of the virtual function (as it's virtual, it's not immediately known at compile time), which could go wrong. If it's not virtual, the function IS known at compile time, and it's simply called with an invalid "this" pointer. It's still likely it will crash when something of the class is actually read from or written to, though.

Originally Posted by EVOEx

My guess is that

Code:

ch->GetState()

returns an invalid pointer. It uses this pointer to find out the address of the virtual function (as it's virtual, it's not immediately known at compile time), which could go wrong. If it's not virtual, the function IS known at compile time, and it's simply called with an invalid "this" pointer. It's still likely it will crash when something of the class is actually read from or written to, though.

That does seem to be the issue. Putting a guard like "if(ch->GetState())" stops the crash (and the program from functioning 8D).


To my reckoning, the following program I whipped up is pretty much how I want the other, actual program to work. However, I can't reproduce the same error.

Code:

#include <iostream>

class ConnectionState
{
int number;
public:
ConnectionState() {number=0;}
	
	virtual void ProcessInput(int n);

};

void ConnectionState::ProcessInput(int n)
{
	std::cout<<"ProcessInput() called.\n";
	number=n;
	std::cout<<"number set to "<<number<<".\n";
}

class Player
{
ConnectionState* state;
public:
Player(ConnectionState* p):state(p) {}
ConnectionState* GetState() {return state;}
};

namespace states{
ConnectionState gState;
};

int main()
{
Player* a_player = new Player(&states::gState);
a_player->GetState()->ProcessInput(5);

delete a_player;
return 0;
}

Could you copy the GetState function and the places this state (assuming it's similar to what you have written in your last post) actually gets assigned?

From what you've copied, I can imagine one problem. If you have two constructors, one with a parameter and one without a parameter, where you assign the state in the one with a parameter only. In this case, if you call the constructor without a parameter, it won't be assigned and the value of "state" can be anything?
If that's not the problem, copy the GetState function and the places "state" is assigned and/or modified (the pointer, not the class instance itself).

I think I found out what the problem was.
Changing the line that caused the crash to

Code:

ch->GetState()->pdConState::ProcessUserInput(ch);

makes the code work as expected.

I'm still trying to wrap my head around it, but I think the jist of the problem is that you need to suppress the virtual call mechanism if you want to call a virtual class defined in the base class. Or something. (It still doesn't really explain why the "demo" program I put in my last post works without it).

The explanation I am reading is at http://publib.boulder.ibm.com/infoce...ef/cplr139.htm.

Edited to quote the part of the above link I think is relevant:

If a function is declared virtual in its base class, you can still access it directly using the scope resolution (: operator. In this case, the virtual function call mechanism is suppressed and the function implementation defined in the base class is used. In addition, if you do not override a virtual member function in a derived class, a call to that function uses the function implementation defined in the base class.

I don't think it actually works. I think you're lucky that it seems to work on your specific system.

See, it's exactly what I explained earlier. If you use a virtual function, the compiler doesn't know yet what function to call. It uses the pointer of the object to find out the address of this function and then runs this code. Your application crashes because this pointer can't be read or because the function pointer is invalid.
Now you did the same as not making the function virtual. Now the compiler DOES know what function to call, and it won't have to use this "virtual table" to find out. It will simply run the routine, and assign the invalid pointer to the "this" pointer. Meaning that any time you read from or write to class instance memory, it will crash or read/write wrong data.

Don't use the fix. You've simply made the virtual keyword completely useless. Fix the underlying problem; the invalid pointer that is returned by the GetState function.

Do you override ProcessUserInput in any derived class? Are you sure that the implementation of such an override is correct?

Originally Posted by laserlight

Do you override ProcessUserInput in any derived class? Are you sure that the implementation of such an override is correct?

Well, in that case the bold line wouldn't be the place of the crash but rather it would crash inside the called function. Also, adding "if(ch->GetState())" wouldn't stop the crash from happening.

Originally Posted by EVOEx

Well, in that case the bold line wouldn't be the place of the crash but rather it would crash inside the called function. Also, adding "if(ch->GetState())" wouldn't stop the crash from happening.

That's true, but I am also looking to understand why Boxknife wants to declare the function virtual in the first place, and yet can freely "undeclare" its virtual aspect.

Don't use the fix. You've simply made the virtual keyword completely useless. Fix the underlying problem; the invalid pointer that is returned by the GetState function.

You're right. The whole idea was to have it set up so that I could make different connection states derived from that class to assign, and resolving the scope to the base class every time defeats the purpose. I'm trying to figure out what's borking the pointer up.

Do you override ProcessUserInput in any derived class? Are you sure that the implementation of such an override is correct?

I don't have any classes derived from it yet.

What I think I'm going to try to do is to make that base class an abstract class and only use classes derived from it.

Boxknife, the problem is not even that the virtual keyword was useless there. I've written three examples to show you the actual problem. The cout's tell you what should have happened (it could've been comments, but now there's some more code). But this happens ON MY computer/compiler, but I reckon it will happen on most compilers:

First, your crashing example:

Code:

#include <iostream>

struct A {
	virtual void hello()
	{
		std::cout << "Never going to reach this!" << std::endl;
	}
};

A *getBadPointer()
{
	return NULL;
}

int main()
{
	getBadPointer()->hello();
}

I've explained two times already why this crashed. If you still don't get it, read my posts again. So now I added your hack fix, which doesn't crash (here, at least):

Code:

#include <iostream>

struct A {
	virtual void hello()
	{
		std::cout << "I'm not going to crash here!" << std::endl;
	}
};

A *getBadPointer()
{
	return NULL;
}

int main()
{
	getBadPointer()->A::hello();
}

I've also explained why this doesn't crash. Finally, however, I modify the hello function to show you that a simple change in the function DOES make it crash again:

Code:

#include <iostream>

struct A {
	int someVar;

	virtual void hello()
	{
		std::cout << "I'm still alive, but not for long!" << std::endl;
		someVar = 1;
		std::cout << "And I'll never even reach this code..." << std::endl;
	}
};

A *getBadPointer()
{
	return NULL;
}

int main()
{
	getBadPointer()->A::hello();
}

And that's the real problem with your "fix".


Anyways, yes, find out where the bad pointer comes from. If you need help, post the code where the "state" variable is assigned.


EDIT: YES, I *do* put the "*" for pointers next to the variable/function name rather than the type. I started out at C. And I still think it's more readable. Sue me.

Funnily enough, I'd forgotten to copy the state pointer in the copy constructor that was being used (though I'd gotten it in the regular constructor and the overloaded assignment operator - those were the first places I looked). I'd forgotten I'd even made one at all.

Also, just to be clear: yes, I understood you both times, and I appreciate your help.

Dereferencing a borked/NULL pointer was my first suspicion, but I mistakenly decided to look for other possible causes after checking what I thought were the class's only two constructors.

(Edited to finally post the code where the state variable is assigned) :3

Code:

	pdPlayer::pdPlayer(const pdPlayer& p):pdCharacter("", "", "", "", GENDER_N)
	{
		requestdisconnect=p.requestdisconnect;
		readbufferdirty=p.readbufferdirty;
		writebufferdirty=p.writebufferdirty;
		writebuffer=p.writebuffer;
		cmdbuffer=p.cmdbuffer;
		player_fd=p.player_fd;
		player_accepted=p.player_accepted;
		player_address=p.player_address;
+		state=p.state;
	}