[Bug Fix] Fix miscellaneous memory leaks related to EQApplicationPacket and it's pBuffer (#2262)

* Delete EQApplicationPacket after use

* Correct issue where Creating EQApplicationPackets in zone/mob.cpp may not free pBuffer[]

* Handle freeing pBuffer[] if exists in zone/mob.cpp Create*Packet functions

* Handle freeing pBuffer[] in zone/object.cpp Create*Packet methods

* Delete leaked outapp variables in zone/trading.cpp

* Make CreateHorseSpawnPacket() safer by freeing pBuffer[] before replacing with new[]

* Prevent initial new ServerPacket from being leaked in command_reload

* Free pack after sending in command_setlsinfo

* Delete new NPC in command_viewnpctype after printing stats

* Fix compile error

* Delete EQApplicationPacket after sending in command_kick

* Remove unneeded safe_delete(outapp) after FastQueuePacket and fix minor whitespace issue

* Delete packet after sending in WorldServer::SendReloadTasks

* Cleanup logic and free leaked NPCType in Client::Doppelganger

* Free RespawnOption in Client::HandleRespawnFromHover in 'unexpected rez from hover request' branch

* Free EQApplicationPacket after sending in Bot::ProcessBotInspectionRequest

* Free Bot when returning from failed Bot::Save() in helper_bot_create()

* Initialize variable to nullptr to prevent garbage initial value

* Undo change in other PR

zone/bot.cpp

@@ -8797,6 +8797,7 @@ void Bot::ProcessBotInspectionRequest(Bot* inspectedBot, Client* client) {
 		strcpy(insr->text, inspectedBot->GetInspectMessage().text);
 
 		client->QueuePacket(outapp); // Send answer to requester
+		safe_delete(outapp);
 	}
 }
 

zone/bot_command.cpp

@@ -5123,7 +5123,7 @@ void bot_subcommand_bot_clone(Client *c, const Seperator *sep)
 }
 
 void bot_command_view_combos(Client *c, const Seperator *sep)
-{	
+{
 	const std::string class_substrs[17] = { "",
 		"%u (WAR)", "%u (CLR)", "%u (PAL)", "%u (RNG)",
 		"%u (SHD)", "%u (DRU)", "%u (MNK)", "%u (BRD)",
@@ -5375,7 +5375,7 @@ void bot_subcommand_bot_dye_armor(Client *c, const Seperator *sep)
 		EQ::textures::armorLegs,
 		EQ::textures::armorFeet
 	);
-	
+
 	if (helper_command_alias_fail(c, "bot_subcommand_bot_dye_armor", sep->arg[0], "botdyearmor")) {
 		return;
 	}
@@ -5943,7 +5943,7 @@ void bot_subcommand_bot_list(Client *c, const Seperator *sep)
 			false,
 			bots_iter.Name
 		);
-	
+
 		c->Message(
 			Chat::White,
 			fmt::format(
@@ -6333,7 +6333,7 @@ void bot_subcommand_bot_stance(Client *c, const Seperator *sep)
 
 	bool current_flag = false;
 	auto bst = EQ::constants::stanceUnknown;
-	
+
 	if (!strcasecmp(sep->arg[1], "current"))
 		current_flag = true;
 	else if (sep->IsNumber(1)) {
@@ -7317,7 +7317,7 @@ void bot_subcommand_botgroup_list(Client *c, const Seperator *sep)
 				(botgroup_count + 1),
 				botgroups_iter.first,
 				botgroups_iter.second,
-				database.botdb.IsBotGroupAutoSpawn(botgroups_iter.first) ? " (Auto Spawn)" : "",				
+				database.botdb.IsBotGroupAutoSpawn(botgroups_iter.first) ? " (Auto Spawn)" : "",
 				EQ::SayLinkEngine::GenerateQuestSaylink(
 					fmt::format("^botgroupload {}", botgroups_iter.first),
 					false,
@@ -9389,6 +9389,7 @@ uint32 helper_bot_create(Client *bot_owner, std::string bot_name, uint8 bot_clas
 
 	if (!my_bot->Save()) {
 		bot_owner->Message(Chat::White, "Failed to create '%s' due to unknown cause", my_bot->GetCleanName());
+		safe_delete(my_bot);
 		return bot_id;
 	}
 

zone/client.cpp

@@ -6517,8 +6517,6 @@ void Client::Doppelganger(uint16 spell_id, Mob *target, const char *name_overrid
 	}
 	made_npc->loottable_id = 0;
 
-	npc_type = made_npc;
-
 	int summon_count = pet.count;
 
 	if(summon_count > MAX_SWARM_PETS)
@@ -6531,14 +6529,11 @@ void Client::Doppelganger(uint16 spell_id, Mob *target, const char *name_overrid
 	};
 
 	while(summon_count > 0) {
-		NPCType *npc_dup = nullptr;
+		auto npc_type_copy = new NPCType;
-		if(made_npc != nullptr) {
+		memcpy(npc_type_copy, made_npc, sizeof(NPCType));
-			npc_dup = new NPCType;
-			memcpy(npc_dup, made_npc, sizeof(NPCType));
-		}
 
 		NPC* swarm_pet_npc = new NPC(
-				(npc_dup!=nullptr)?npc_dup:npc_type,	//make sure we give the NPC the correct data pointer
+				npc_type_copy,
 				0,
 				GetPosition() + glm::vec4(swarmPetLocations[summon_count - 1], 0.0f, 0.0f),
 				GravityBehavior::Water);
@@ -6563,12 +6558,13 @@ void Client::Doppelganger(uint16 spell_id, Mob *target, const char *name_overrid
 		swarm_pet_npc->GetSwarmInfo()->target = 0;
 
 		//we allocated a new NPC type object, give the NPC ownership of that memory
-		if(npc_dup != nullptr)
+		swarm_pet_npc->GiveNPCTypeData(npc_type_copy);
-			swarm_pet_npc->GiveNPCTypeData(npc_dup);
 
 		entity_list.AddNPC(swarm_pet_npc);
 		summon_count--;
 	}
+
+	safe_delete(made_npc);
 }
 
 void Client::AssignToInstance(uint16 instance_id)

zone/client_process.cpp

@@ -2053,6 +2053,7 @@ void Client::HandleRespawnFromHover(uint32 Option)
 			if (PendingRezzXP < 0 || PendingRezzSpellID == 0)
 			{
 				LogSpells("Unexpected Rezz from hover request");
+				safe_delete(default_to_bind);
 				return;
 			}
 			SetHP(GetMaxHP() / 5);

zone/gm_commands/kick.cpp

@@ -17,6 +17,7 @@ void command_kick(Client *c, const Seperator *sep)
 		if (client->Admin() <= c->Admin()) {
 			auto outapp = new EQApplicationPacket(OP_GMKick, 0);
 			client->QueuePacket(outapp);
+			safe_delete(outapp);
 			client->Kick("Ordered kicked by command");
 			c->Message(
 				Chat::White,

zone/gm_commands/object.cpp

@@ -1122,8 +1122,8 @@ void command_object(Client *c, const Seperator *sep)
 			app = new EQApplicationPacket();
 			o->CreateDeSpawnPacket(app);
 			entity_list.QueueClients(nullptr, app);
-
 			entity_list.RemoveObject(o->GetID());
+			safe_delete(app);
 
 			// Verifying ZoneID and Version in case someone else ended up adding an object with our ID
 			// from a different zone/version. Don't want to delete someone else's work.

zone/gm_commands/reload.cpp

@@ -61,7 +61,7 @@ void command_reload(Client *c, const Seperator *sep)
 		return;
 	}
 
-	auto pack = new ServerPacket;
+	ServerPacket* pack = nullptr;
 
 	if (is_aa) {
 		c->Message(Chat::White, "Attempting to reload Alternate Advancement Data globally.");	
@@ -282,7 +282,7 @@ void command_reload(Client *c, const Seperator *sep)
 		pack = new ServerPacket(ServerOP_ReloadZonePoints, 0);
 	}
 	
-	if (pack->opcode) {
+	if (pack) {
 		worldserver.SendPacket(pack);
 	}
 

zone/gm_commands/setlsinfo.cpp

@@ -18,6 +18,7 @@ void command_setlsinfo(Client *c, const Seperator *sep)
 	strn0cpy(s->user_email, sep->arg[1], 100);
 	strn0cpy(s->userpassword, sep->arg[2], 50);
 	worldserver.SendPacket(pack);
+	safe_delete(pack);
 	c->Message(Chat::White, "Your email and local loginserver password have been set.");
 }
 

zone/gm_commands/viewnpctype.cpp

@@ -13,6 +13,7 @@ void command_viewnpctype(Client *c, const Seperator *sep)
 				GravityBehavior::Water
 			);
 			npc->ShowStats(c);
+			safe_delete(npc);
 		}
 		else {
 			c->Message(

zone/horse.cpp

@@ -174,6 +174,7 @@ void Client::SetHorseId(uint16 horseid_in) {
 
 void Mob::CreateHorseSpawnPacket(EQApplicationPacket* app, const char* ownername, uint16 ownerid, Mob* ForWho) {
 	app->SetOpcode(OP_NewSpawn);
+	safe_delete_array(app->pBuffer);
 	app->pBuffer = new uchar[sizeof(NewSpawn_Struct)];
 	app->size = sizeof(NewSpawn_Struct);
 	memset(app->pBuffer, 0, sizeof(NewSpawn_Struct));

zone/mob.cpp

@@ -1158,6 +1158,7 @@ void Mob::CreateSpawnPacket(EQApplicationPacket *app, Mob *ForWho)
 {
 	app->SetOpcode(OP_NewSpawn);
 	app->size = sizeof(NewSpawn_Struct);
+	safe_delete_array(app->pBuffer);
 	app->pBuffer = new uchar[app->size];
 	memset(app->pBuffer, 0, app->size);
 	auto ns = (NewSpawn_Struct *) app->pBuffer;
@@ -1175,7 +1176,7 @@ void Mob::CreateSpawnPacket(EQApplicationPacket *app, Mob *ForWho)
 void Mob::CreateSpawnPacket(EQApplicationPacket* app, NewSpawn_Struct* ns) {
 	app->SetOpcode(OP_NewSpawn);
 	app->size = sizeof(NewSpawn_Struct);
-
+	safe_delete_array(app->pBuffer);
 	app->pBuffer = new uchar[sizeof(NewSpawn_Struct)];
 
 	// Copy ns directly into packet
@@ -1367,6 +1368,7 @@ void Mob::CreateDespawnPacket(EQApplicationPacket* app, bool Decay)
 {
 	app->SetOpcode(OP_DeleteSpawn);
 	app->size = sizeof(DeleteSpawn_Struct);
+	safe_delete_array(app->pBuffer);
 	app->pBuffer = new uchar[app->size];
 	memset(app->pBuffer, 0, app->size);
 	DeleteSpawn_Struct* ds = (DeleteSpawn_Struct*)app->pBuffer;
@@ -1380,6 +1382,7 @@ void Mob::CreateHPPacket(EQApplicationPacket* app)
 	IsFullHP=(current_hp>=max_hp);
 	app->SetOpcode(OP_MobHealth);
 	app->size = sizeof(SpawnHPUpdate_Struct2);
+	safe_delete_array(app->pBuffer);
 	app->pBuffer = new uchar[app->size];
 	memset(app->pBuffer, 0, sizeof(SpawnHPUpdate_Struct2));
 	SpawnHPUpdate_Struct2* ds = (SpawnHPUpdate_Struct2*)app->pBuffer;

zone/object.cpp

@@ -412,6 +412,7 @@ EQ::ItemInstance* Object::PopItem(uint8 index)
 void Object::CreateSpawnPacket(EQApplicationPacket* app)
 {
 	app->SetOpcode(OP_GroundSpawn);
+	safe_delete_array(app->pBuffer);
 	app->pBuffer = new uchar[sizeof(Object_Struct)];
 	app->size = sizeof(Object_Struct);
 	memcpy(app->pBuffer, &m_data, sizeof(Object_Struct));
@@ -420,6 +421,7 @@ void Object::CreateSpawnPacket(EQApplicationPacket* app)
 void Object::CreateDeSpawnPacket(EQApplicationPacket* app)
 {
 	app->SetOpcode(OP_ClickObject);
+	safe_delete_array(app->pBuffer);
 	app->pBuffer = new uchar[sizeof(ClickObject_Struct)];
 	app->size = sizeof(ClickObject_Struct);
 	memset(app->pBuffer, 0, sizeof(ClickObject_Struct));

zone/trading.cpp

@@ -1078,6 +1078,7 @@ void Client::Trader_CustomerBrowsing(Client *Customer) {
 	sis->TraderID = Customer->GetID();
 
 	QueuePacket(outapp);
+	safe_delete(outapp);
 }
 
 
@@ -1151,8 +1152,8 @@ void Client::Trader_EndTrader() {
 			}
 
 			safe_delete(outapp);
+			safe_delete(gis);
 		}
-		safe_delete(gis);
 	}
 
 	database.DeleteTraderItem(CharacterID());
@@ -2584,6 +2585,7 @@ void Client::ShowBuyLines(const EQApplicationPacket *app) {
 		VARSTRUCT_ENCODE_STRING(Buf, Buyer->GetName());
 
 		QueuePacket(outapp);
+		safe_delete(outapp);
     }
 }
 

zone/worldserver.cpp

@@ -3416,6 +3416,7 @@ void WorldServer::SendReloadTasks(uint8 reload_type, uint32 task_id) {
 	rts->task_id = task_id;
 
 	SendPacket(pack);
+	safe_delete(pack);
 }
 
 void WorldServer::HandleReloadTasks(ServerPacket *pack)

zone/zoning.cpp

@@ -491,21 +491,21 @@ void Client::DoZoneSuccess(ZoneChange_Struct *zc, uint16 zone_id, uint32 instanc
 
 		zone->StartShutdownTimer(AUTHENTICATION_TIMEOUT * 1000);
 	} else {
-	// vesuvias - zoneing to another zone so we need to the let the world server
+		// vesuvias - zoneing to another zone so we need to the let the world server
-	//handle things with the client for a while
+		//handle things with the client for a while
-	auto pack = new ServerPacket(ServerOP_ZoneToZoneRequest, sizeof(ZoneToZone_Struct));
+		auto pack = new ServerPacket(ServerOP_ZoneToZoneRequest, sizeof(ZoneToZone_Struct));
-	ZoneToZone_Struct *ztz = (ZoneToZone_Struct *)pack->pBuffer;
+		ZoneToZone_Struct *ztz = (ZoneToZone_Struct *)pack->pBuffer;
-	ztz->response = 0;
+		ztz->response = 0;
-	ztz->current_zone_id = zone->GetZoneID();
+		ztz->current_zone_id = zone->GetZoneID();
-	ztz->current_instance_id = zone->GetInstanceID();
+		ztz->current_instance_id = zone->GetInstanceID();
-	ztz->requested_zone_id = zone_id;
+		ztz->requested_zone_id = zone_id;
-	ztz->requested_instance_id = instance_id;
+		ztz->requested_instance_id = instance_id;
-	ztz->admin = admin;
+		ztz->admin = admin;
-	ztz->ignorerestrictions = ignore_r;
+		ztz->ignorerestrictions = ignore_r;
-	strcpy(ztz->name, GetName());
+		strcpy(ztz->name, GetName());
-	ztz->guild_id = GuildID();
+		ztz->guild_id = GuildID();
-	worldserver.SendPacket(pack);
+		worldserver.SendPacket(pack);
-	safe_delete(pack);
+		safe_delete(pack);
 	}
 
 	//reset to unsolicited.
@@ -778,7 +778,6 @@ void Client::ZonePC(uint32 zoneID, uint32 instance_id, float x, float y, float z
 
 			outapp->priority = 6;
 			FastQueuePacket(&outapp);
-			safe_delete(outapp);
 		}
 		else if(zm == ZoneSolicited || zm == ZoneToSafeCoords) {
 			auto outapp =
@@ -795,7 +794,6 @@ void Client::ZonePC(uint32 zoneID, uint32 instance_id, float x, float y, float z
 
 			outapp->priority = 6;
 			FastQueuePacket(&outapp);
-			safe_delete(outapp);
 		}
 		else if(zm == EvacToSafeCoords) {
 			auto outapp =
@@ -829,7 +827,6 @@ void Client::ZonePC(uint32 zoneID, uint32 instance_id, float x, float y, float z
 
 			outapp->priority = 6;
 			FastQueuePacket(&outapp);
-			safe_delete(outapp);
 		}
 		else {
 			if(zoneID == GetZoneID()) {
@@ -854,7 +851,6 @@ void Client::ZonePC(uint32 zoneID, uint32 instance_id, float x, float y, float z
 			gmg->type = 0x01;	//an observed value, not sure of meaning
 			outapp->priority = 6;
 			FastQueuePacket(&outapp);
-			safe_delete(outapp);
 		}
 
 		LogDebug("Player [{}] has requested a zoning to LOC x=[{}], y=[{}], z=[{}], heading=[{}] in zoneid=[{}]", GetName(), x, y, z, heading, zoneID);