[Bug]: Potential followers crash

[maattch]

By submitting this bug issue, you agree to the following.

• This is a bug in the software that resides in this repository, and not a support matter (use https://otland.net/forums/support.16/ for support)
• This issue is reproducible without changes to the C++ code in this repository
• This bug has not been resolved in master branch
• There is no existing issue for this bug already

Does this bug crash tfs?

not sure

Server Version

1.7 (Master)

Operation System

all (listed below)

OS Description

No response

Bug description

Following creature can cause crash

Possible Pull Requests which are to blame

• Optimize pathfinding #4637

Steps to reproduce

I could not reproduce a crash but just removing or killing the creature following you and walking away could potentialy crash.

Actual Behavior

When a creature follows you, it is added to your followers list, which is updated every time you walk removing creatures that are too far from you. However, when the creature in the list is removed or killed, it is not removed from the list, causing a dangling pointer.

forgottenserver/src/creature.cpp

Lines 838 to 841 in c519cc6

	const Position& followerPosition = creature->getPosition();
	uint16_t distance = position.getDistanceX(followerPosition) +
	position.getDistanceY(followerPosition);
	return distance >= Map::maxViewportX + Map::maxViewportY ||

I couldn't reproduce a crash, but the debugger reported corrupted positions on that line (e.g x=42678, y=39234, z=219), confirming that it's indeed a dangling pointer.

Expected Behavior

Not crash?

Backtrace

[ArturKnopik]

It's looks like possible nullptr dereference

[NRH-AA]

Probably just need to replace:

creature.cpp

void Creature::removeFollowers()
{
	const Position& position = getPosition();

	followers.erase(std::remove_if(followers.begin(), followers.end(),
	                               [&position](Creature* creature) {
		                               const Position& followerPosition = creature->getPosition();
		                               uint16_t distance = position.getDistanceX(followerPosition) +
		                                                   position.getDistanceY(followerPosition);
		                               return distance >= Map::maxViewportX + Map::maxViewportY ||
		                                      position.z != followerPosition.z;
	                               }),
	                followers.end());
}

with

void Creature::removeFollowers()
{
	const Position& position = getPosition();

	followers.erase(std::remove_if(followers.begin(), followers.end(),
	                               [&position](Creature* creature) {
		                               if (!creature) {
			                               return true;
		                               }
		                               const Position& followerPosition = creature->getPosition();
		                               uint16_t distance = position.getDistanceX(followerPosition) +
		                                                   position.getDistanceY(followerPosition);
		                               return distance >= Map::maxViewportX + Map::maxViewportY ||
		                                      position.z != followerPosition.z;
	                               }),
	                followers.end());
}

Can you check it out? It probably didn't crash because the positions are so out of whack it removed the creature anyway but can definitely cause a crash in the right case.

[ArturKnopik]

Probably just need to replace:

creature.cpp

void Creature::removeFollowers()
{
	const Position& position = getPosition();

	followers.erase(std::remove_if(followers.begin(), followers.end(),
	                               [&position](Creature* creature) {
		                               const Position& followerPosition = creature->getPosition();
		                               uint16_t distance = position.getDistanceX(followerPosition) +
		                                                   position.getDistanceY(followerPosition);
		                               return distance >= Map::maxViewportX + Map::maxViewportY ||
		                                      position.z != followerPosition.z;
	                               }),
	                followers.end());
}

with

void Creature::removeFollowers()
{
	const Position& position = getPosition();

	followers.erase(std::remove_if(followers.begin(), followers.end(),
	                               [&position](Creature* creature) {
		                               if (!creature) {
			                               return true;
		                               }
		                               const Position& followerPosition = creature->getPosition();
		                               uint16_t distance = position.getDistanceX(followerPosition) +
		                                                   position.getDistanceY(followerPosition);
		                               return distance >= Map::maxViewportX + Map::maxViewportY ||
		                                      position.z != followerPosition.z;
	                               }),
	                followers.end());
}

Can you check it out? It probably didn't crash because the positions are so out of whack it removed the creature anyway but can definitely cause a crash in the right case.

fix looks good, I wonder if there is a chance for nullptr in the list, any new that failed should throw exception that kill server

[maattch]

I don't think checking for nullptr will help, when the creature is deleted the pointer in the vector can still be "valid". I guess the best solution is to increment reference counter when you add a follower, and decrement when removed. This way we can just check if the creature is removed (creature->isRemoved())

[ArturKnopik]

it's true, i almost forgot tfs i using raw ptr..
moving to smart ptrs is realy big task(thing class tree)

[NRH-AA]

I don't think checking for nullptr will help, when the creature is deleted the pointer in the vector can still be "valid". I guess the best solution is to increment reference counter when you add a follower, and decrement when removed. This way we can just check if the creature is removed (creature->isRemoved())

I am still not sure how reference counter actually works. How would increasing/decreasing a number determine if a creature is there or not? Couldn't the number be lowered and it isn't the exact creature we are looking for? I haven't had the time to really understand what the reference counter is doing so maybe I can get some pointers from you?

If the creature is removed (dead, ect) how would the pointer in the followers vector still be valid?

This is my guess at this point:
If the creature's reference counter is 0 than we can assume that the creature is gone? Which this is due to not using smart pointers?

Decreasing the ref count to remove the follower is implicitly not going to work if I am not mistaken? We are already trying to determine if the creature is not there anymore which is causing the problem. So we cant decrement it if we can't figure out if it is gone in the first place right?

[ArturKnopik]

increment/decrement reff count is for self memory managment, if counter reach 0 is call delete on this, it's should be replaced by smart ptr and increment/decrement will be no longer needed, i disslike this pattern, it's hard to follow when thing is removed

[NRH-AA]

So the only option I could see is just changing this:

if (!creature) {
	 return true;
 }

to

if (creature->isRemoved()) {
	 return true;
 }

What am I missing here?

[maattch]

So the only option I could see is just changing this:

if (!creature) {
	 return true;
 }

to

if (creature->isRemoved()) {
	 return true;
 }

What am I missing here?

That's right, but you will need to call creature->decrementReferenceCounter() for every creature removed from the list, and creature->incrementReferenceCounter() when you add a creature in the list.

[NRH-AA]

That's right, but you will need to call creature->decrementReferenceCounter() for every creature removed from the list, and creature->incrementReferenceCounter() when you add a creature in the list.

I think I get it. Because we increment in the followers it wont be deleted until we remove the follower. Will that cause any other problems like hanging creatures? If we don't remove it as a follower as soon as it dies wont it stick around until it is removed?