Fixed autobonus removal (#3767)

Fixes #3739

Thanks to @whupdo for finding the broken removal.

Fixes #3766

Since vector elements will be pushed around in the container when an element is removed,
the original pointer might be invalid at the time the end of the timer has been reached.

A big thank you to @teededung for being persistent and helping me to reproduce the issue.

src/map/pc.cpp

@@ -2356,10 +2356,14 @@ void pc_delautobonus(struct map_session_data* sd, std::vector<s_autobonus> &bonu
 	if (!sd)
 		return;
 
-	for (uint8 i = 0; i < bonus.size(); i++) {
+	std::vector<s_autobonus>::iterator it = bonus.begin();
-		if (bonus[i].active != INVALID_TIMER) {
+
-			if (restore && (sd->state.autobonus&bonus[i].pos) == bonus[i].pos) {
+	while( it != bonus.end() ){
-				if (bonus[i].bonus_script) {
+		s_autobonus b = *it;
+
+		if (b.active != INVALID_TIMER) {
+			if (restore && (sd->state.autobonus&b.pos) == b.pos) {
+				if (b.bonus_script) {
 					unsigned int equip_pos_idx = 0;
 
 					// Create a list of all equipped positions to see if all items needed for the autobonus are still present [Playtester]
@@ -2368,22 +2372,26 @@ void pc_delautobonus(struct map_session_data* sd, std::vector<s_autobonus> &bonu
 							equip_pos_idx |= sd->inventory.u.items_inventory[sd->equip_index[j]].equip;
 					}
 
-					if ((equip_pos_idx&bonus[i].pos) == bonus[i].pos)
+					if ((equip_pos_idx&b.pos) == b.pos)
-						script_run_autobonus(bonus[i].bonus_script, sd, bonus[i].pos);
+						script_run_autobonus(b.bonus_script, sd, b.pos);
 				}
+
+				it++;
+
 				continue;
 			} else { // Logout / Unequipped an item with an activated bonus
-				delete_timer(bonus[i].active, pc_endautobonus);
+				delete_timer(b.active, pc_endautobonus);
-				bonus[i].active = INVALID_TIMER;
+				b.active = INVALID_TIMER;
 			}
 		}
 
-		if (bonus[i].bonus_script)
+		if (b.bonus_script)
-			aFree(bonus[i].bonus_script);
+			aFree(b.bonus_script);
-		if (bonus[i].other_script)
+		if (b.other_script)
-			aFree(bonus[i].other_script);
+			aFree(b.other_script);
+
+		it = bonus.erase(it);
 	}
-	bonus.clear();
 }
 
 /**
@@ -2391,7 +2399,7 @@ void pc_delautobonus(struct map_session_data* sd, std::vector<s_autobonus> &bonu
  * @param sd: Player data
  * @param autobonus: Autobonus to run
  */
-void pc_exeautobonus(struct map_session_data *sd, struct s_autobonus *autobonus)
+void pc_exeautobonus(struct map_session_data *sd, std::vector<s_autobonus> *bonus, struct s_autobonus *autobonus)
 {
 	if (!sd || !autobonus)
 		return;
@@ -2412,7 +2420,7 @@ void pc_exeautobonus(struct map_session_data *sd, struct s_autobonus *autobonus)
 			script_run_autobonus(autobonus->other_script,sd,autobonus->pos);
 	}
 
-	autobonus->active = add_timer(gettick()+autobonus->duration, pc_endautobonus, sd->bl.id, (intptr_t)autobonus);
+	autobonus->active = add_timer(gettick()+autobonus->duration, pc_endautobonus, sd->bl.id, (intptr_t)bonus);
 	sd->state.autobonus |= autobonus->pos;
 	status_calc_pc(sd,SCO_FORCE);
 }
@@ -2422,13 +2430,19 @@ void pc_exeautobonus(struct map_session_data *sd, struct s_autobonus *autobonus)
  */
 TIMER_FUNC(pc_endautobonus){
 	struct map_session_data *sd = map_id2sd(id);
-	struct s_autobonus *autobonus = (struct s_autobonus *)data;
+	std::vector<s_autobonus> *bonus = (std::vector<s_autobonus> *)data;
 
 	nullpo_ret(sd);
-	nullpo_ret(autobonus);
+	nullpo_ret(bonus);
 
-	autobonus->active = INVALID_TIMER;
+	for( struct s_autobonus& autobonus : *bonus ){
-	sd->state.autobonus &= ~autobonus->pos;
+		if( autobonus.active == tid ){
+			autobonus.active = INVALID_TIMER;
+			sd->state.autobonus &= ~autobonus.pos;
+			break;
+		}
+	}
+	
 	status_calc_pc(sd,SCO_FORCE);
 	return 0;
 }

src/map/pc.hpp

@@ -1068,7 +1068,7 @@ bool pc_adoption(struct map_session_data *p1_sd, struct map_session_data *p2_sd,
 void pc_updateweightstatus(struct map_session_data *sd);
 
 bool pc_addautobonus(std::vector<s_autobonus> &bonus, const char *script, short rate, unsigned int dur, short atk_type, const char *o_script, unsigned int pos, bool onskill);
-void pc_exeautobonus(struct map_session_data* sd, struct s_autobonus *bonus);
+void pc_exeautobonus(struct map_session_data* sd, std::vector<s_autobonus> *bonus, struct s_autobonus *autobonus);
 TIMER_FUNC(pc_endautobonus);
 void pc_delautobonus(struct map_session_data* sd, std::vector<s_autobonus> &bonus, bool restore);
 

src/map/skill.cpp

@@ -2179,7 +2179,7 @@ int skill_additional_effect(struct block_list* src, struct block_list *bl, uint1
 				  ((it.atk_type)&attack_type)&BF_RANGEMASK &&
 				  ((it.atk_type)&attack_type)&BF_SKILLMASK))
 				continue; // one or more trigger conditions were not fulfilled
-			pc_exeautobonus(sd, &it);
+			pc_exeautobonus(sd, &sd->autobonus, &it);
 		}
 	}
 
@@ -2279,7 +2279,7 @@ int skill_onskillusage(struct map_session_data *sd, struct block_list *bl, uint1
 				continue;
 			if (it.atk_type != skill_id)
 				continue;
-			pc_exeautobonus(sd, &it);
+			pc_exeautobonus(sd, &sd->autobonus3, &it);
 		}
 	}
 
@@ -2510,7 +2510,7 @@ int skill_counter_additional_effect (struct block_list* src, struct block_list *
 				  ((it.atk_type)&attack_type)&BF_RANGEMASK &&
 				  ((it.atk_type)&attack_type)&BF_SKILLMASK))
 				continue; // one or more trigger conditions were not fulfilled
-			pc_exeautobonus(dstsd, &it);
+			pc_exeautobonus(dstsd, &dstsd->autobonus2, &it);
 		}
 	}