[C++] Fishing: forged casts accepted anywhere, a reload crash and fish consumed for nothing

[Zektq 4]

Hi,

While reworking fishing on our server we went through the stock fishing code (fishing.cpp, char.cpp, input_main.cpp) and found a few bugs. They're in the original game source, so most files should have them. Function names and lines can differ on yours: search for the snippets.

1. Core crash on boot or on /reload f

If the last line of fishing.txt has no line break, strrchr returns NULL and the core writes to it.

fishing.cpp, in Initialize(), search:

		char * p = strrchr(buf, '\n');
		*p = '\0';

Replace with:

		char * p = strrchr(buf, '\n');
		if (p)
			*p = '\0';

2. FishingPractice uses the rod outside its NULL check

fishing.cpp, at the end of FishingPractice(), search:

	rod->SetSocket(2, 0);
}

Replace with:

	if (rod)
		rod->SetSocket(2, 0);
}

3. GetDesc() used without a check

The fishing event and Take() send a packet with ch->GetDesc()->Packet(...). If the player disconnects at the wrong moment, that's a NULL dereference. In fishing.cpp, search both:

ch->GetDesc()->Packet(&p, sizeof(TPacketGCFishing));

Replace each with:

if (ch->GetDesc())
	ch->GetDesc()->Packet(&p, sizeof(TPacketGCFishing));

4. Fish consumed for nothing (UseFish / Grill)

Both functions deduce the fishing.txt row from the item vnum by subtraction. If a fish item has no row of its own in your fishing.txt (in our files, 27824 to 27832 exist as items but have no row), the index lands on another row: the fish is consumed and the player gets nothing, or the wrong item.

fishing.cpp, in UseFish(), search:

	if (idx<=1 || idx >= MAX_FISH)
		return;

Add after it:

	// the vnum-based index must land on THIS fish's row
	if (fish_info[idx].vnum != item->GetVnum() || !fish_info[idx].dead_vnum)
		return;

In Grill(), search:

	if (idx == -1)
		return;

Add after it:

	// same guard: no row (grill_vnum = 0) means the fish was destroyed for nothing
	if ((fish_info[idx].vnum != vnum && fish_info[idx].dead_vnum != vnum) || !fish_info[idx].grill_vnum)
		return;

5. A forged fishing packet works anywhere

On cast, the stock server only checks the player's own cell (not blocked), the rod and the bait. It even computes the point 4 m in front of the player and never uses it. So a forged HEADER_CG_FISHING fishes in town, in a corner, far from any water, dead or on a horse. The client checks for water before sending, the server never does.

fishing.h, in namespace fishing, add:

	extern bool CanFishNow(LPCHARACTER ch);
	extern bool IsWaterInReach(LPCHARACTER ch);

fishing.cpp, inside namespace fishing, add:

bool CanFishNow(LPCHARACTER ch)
{
	if (!ch)
		return false;

	// what the client blocks before sending, checked again here
	return !ch->IsDead() && !ch->IsStun() && !ch->IsObserverMode() && !ch->IsRiding();
}

bool IsWaterInReach(LPCHARACTER ch)
{
	// The client looks for water 600 units away in the direction it sends
	// (CInstanceBase::GetFishingRot). The angle arrives rounded to 5 degrees and the
	// server position can lag a little, so probe around that point.
	static const float s_afDistance[] = { 600.0f, 500.0f, 700.0f, 400.0f, 800.0f };
	static const float s_afAngle[] = { 0.0f, -5.0f, 5.0f, -10.0f, 10.0f };

	for (size_t a = 0; a < sizeof(s_afAngle) / sizeof(s_afAngle[0]); ++a)
	{
		for (size_t r = 0; r < sizeof(s_afDistance) / sizeof(s_afDistance[0]); ++r)
		{
			float fx, fy;
			GetDeltaByDegree(ch->GetRotation() + s_afAngle[a], s_afDistance[r], &fx, &fy);

			const long x = ch->GetX() + (long) fx;
			const long y = ch->GetY() + (long) fy;

			LPSECTREE tree = SECTREE_MANAGER::instance().Get(ch->GetMapIndex(), x, y);
			if (tree && tree->IsAttr(x, y, ATTR_WATER))
				return true;
		}
	}

	return false;
}

char.cpp, in CHARACTER::fishing(), search:

	{
		LPSECTREE_MAP pkSectreeMap = SECTREE_MANAGER::instance().GetMap(GetMapIndex());

		int	x = GetX();
		int y = GetY();

		LPSECTREE tree = pkSectreeMap->Find(x, y);
		DWORD dwAttr = tree->GetAttribute(x, y);

		if (IS_SET(dwAttr, ATTR_BLOCK))
		{

Replace with:

	if (!fishing::CanFishNow(this))
		return;

	{
		LPSECTREE_MAP pkSectreeMap = SECTREE_MANAGER::instance().GetMap(GetMapIndex());
		if (!pkSectreeMap)
			return;

		int	x = GetX();
		int y = GetY();

		LPSECTREE tree = pkSectreeMap->Find(x, y);
		if (!tree)
			return;

		DWORD dwAttr = tree->GetAttribute(x, y);

		if (IS_SET(dwAttr, ATTR_BLOCK) || !fishing::IsWaterInReach(this))
		{

We compared the server attributes with the client ones on 6 fishing maps: they differ on 63 cells out of about 7 million. We simulated 16,530 legitimate casts with the client's exact rule, and all 16,530 were accepted.

6. Negative cast angles arrive wrapped

The client sends angle / 5 in a BYTE, and GetFishingRot can return negative angles (about 7% of legitimate casts in our test). They arrive wrapped (-20° becomes 252, i.e. 1260°), so other players see the fisher facing another way, and fix 5 would wrongly refuse those casts.

input_main.cpp, in CInputMain::Fishing, search:

	ch->SetRotation(p->dir * 5);

Replace with:

	// the same packet casts and pulls: only the cast carries an angle
	if (!ch->m_pkFishingEvent)
	{
		int iRot = (p->dir >= 128) ? ((int) p->dir - 256) * 5 : (int) p->dir * 5;
		iRot %= 360;
		if (iRot < 0)
			iRot += 360;

		ch->SetRotation((float) iRot);
	}

If m_pkFishingEvent is private in your char.h, add a small getter instead.

All of this is running on our server. If something doesn't match your files, tell me which part and I'll help.

[KolenMG 1157]

thank you for sharing!