event_process (use after free) fixed

[Kuqsal 240]

//Search
int event_process(int pulse)


//Change
int event_process(int pulse)
{
	long	new_time;
	int		num_events = 0;

	while (pulse >= cxx_q.GetTopKey())
	{
		TQueueElement * pElem = cxx_q.Dequeue();

		if (pElem->bCancel)
		{
			cxx_q.Delete(pElem);
			continue;
		}

		new_time = pElem->iKey;

		LPEVENT the_event = pElem->pvData;
		long processing_time = event_processing_time(the_event);
		cxx_q.Delete(pElem);
#ifdef EVENT_PROCESS_HEAP_UP_FREE
		the_event->q_el = NULL;
#endif
		the_event->is_processing = TRUE;

		if (!the_event->info)
		{
			the_event->q_el = NULL;
			ContinueOnFatalError();
		}
		else
		{
			//sys_log(0, "EVENT: %s %d event %p info %p", the_event->file, the_event->line, the_event, the_event->info);
			new_time = (the_event->func) (get_pointer(the_event), processing_time);

			if (new_time <= 0 || the_event->is_force_to_end)
			{
				the_event->q_el = NULL;
			}
			else
			{
				the_event->q_el = cxx_q.Enqueue(the_event, new_time, pulse);
				the_event->is_processing = FALSE;
			}
		}

		++num_events;
	}

	return num_events;
}



//Search
void event_destroy(void)


//Change
void event_destroy(void)
{
	TQueueElement * pElem;

	while ((pElem = cxx_q.Dequeue()))
	{
		LPEVENT the_event = (LPEVENT) pElem->pvData;

		if (!pElem->bCancel)
		{
			// no op here
		}

		cxx_q.Delete(pElem);
#ifdef EVENT_PROCESS_HEAP_UP_FREE
		the_event->q_el = NULL;
#endif
	}
}

 

Edited January 6 by Kuqsal

[Criminus 669]

This feels wrong but I can't prove it. 

[Intel 913]

15 hours ago, Criminus said:

This feels wrong but I can't prove it. 

Nah, it's correct. We detected this bug with asan, too

 

 

Edited February 19 by Metin2 Dev International
Core X - External 2 Internal

[Froslass 851]

15 hours ago, Criminus said:

This feels wrong but I can't prove it. 

He is right, cxx_q.Delete(pElem) frees the queue element, but event->q_el still points to it.

I've looked at the code for a few seconds and from what i saw:
1) event_destroy get's called on server shutdown so we have never experienced a crash because no normal runtime logic accesses events afterward

2) in event_process the_event->is_processing gets set to true, that is a guard-safe to not perform actions on that event while it is true. q_el gets then set to NULL or assigned a new element right below before setting the_event->is_processing to false so the event can get accessed again, which again, kinda explains why we have never experienced a crash even though i can see that event_cancel can still access it during that window so a crash could occur there even if apparently is very unlikely since i've never seen somebody experiencing it

 

Common programming practices on the other hand say that is always better to set a pointer to NULL rather than having a dangling pointer even if we are sure nothing will access that pointer while dangling, because if somebody change some part of the code related to it in the future it will cause a crash

Edit: Ah @ Intel answered right before me i could have saved that time lol

Edited January 12 by Froslass

[Criminus 669]

7 minutes ago, Froslass said:

He is right

Ok, but is this fixing the symptom or the bug? Intel's fix is different. 

[Intel 913]

12 minutes ago, Froslass said:

He is right, cxx_q.Delete(pElem) frees the queue element, but event->q_el still points to it.

I've looked at the code for a few seconds and from what i saw:
1) event_destroy get's called on server shutdown so we have never experienced a crash because no normal runtime logic accesses events afterward

2) in event_process the_event->is_processing gets set to true, that is a guard-safe to not perform actions on that event while it is true. q_el gets then set to NULL or assigned a new element right below before setting the_event->is_processing to false so the event can get accessed again, which again, kinda explains why we have never experienced a crash even though i can see that event_cancel can still access it during that window so a crash could occur there even if apparently is very unlikely since i've never seen somebody experiencing it

 

Common programming practices on the other hand say that is always better to set a pointer to NULL rather than having a dangling pointer even if we are sure nothing will access that pointer while dangling, because if somebody change some part of the code related to it in the future it will cause a crash

Edit: Ah @ Intel answered right before me i could have saved that time lol

Well, as you said, probably nothing was triggering this problem. Our changes to the effect manager made the game kaboom, evidently. Funny thing is now the class manages the timer and not the event queue. Oh well, lol

4 minutes ago, Criminus said:

Ok, but is this fixing the symptom or the bug? Intel's fix is different. 

What? No, it's the same ahah

The changes in event_process and event_destroy are literally the same

Edited January 12 by Intel

[Froslass 851]

3 minutes ago, Criminus said:

Ok, but is this fixing the symptom or the bug? Intel's fix is different. 

What do you mean is different? They are both setting the_event->q_el to null; right after freeing pElem

3 minutes ago, Intel said:

Our changes to the effect manager made the game kaboom

Wait you actually managed to experience this bug? I'm impressed lol

[Criminus 669]

5 minutes ago, Froslass said:

What do you mean is different? They are both setting the_event->q_el to null; right after freeing pElem

You are both right. I am probably too tired. Good night?

[Intel 913]

1 minute ago, Froslass said:

What do you mean is different? They are both setting the_event->q_el to null; right after freeing pElem

Wait you actually managed to experience this bug? I'm impressed lol

Yep, effect_ambience_effect_*, added a param with ttl that would set a timer on how many ms before removing it. There's also another change that we did in event_process, that again, unlikely anyone would encounter it:

		LPEVENT the_event = pElem->pvData;

		if(pElem->bCancel)
		{
			cxx_q.Delete(pElem);

			// if we are processing deletion of the an event's q_el
			// we need to reset q_el as well.
			// This is needed because there might be multiple game's class holding the same
			// LPEVENT and they could try to event_cancel (which resets LPEVENT pointer to NULL) multiple times
			// since event_cancel resets only the caller's pointer
			if (the_event->q_el == pElem) {
				the_event->q_el = nullptr;
			}
			continue;
		}

		new_time = pElem->iKey;

 

[Froslass 851]

5 minutes ago, Intel said:

Yep, effect_ambience_effect_*, added a param with ttl that would set a timer on how many ms before removing it. There's also another change that we did in event_process, that again, unlikely anyone would encounter it:

		LPEVENT the_event = pElem->pvData;

		if(pElem->bCancel)
		{
			cxx_q.Delete(pElem);

			// if we are processing deletion of the an event's q_el
			// we need to reset q_el as well.
			// This is needed because there might be multiple game's class holding the same
			// LPEVENT and they could try to event_cancel (which resets LPEVENT pointer to NULL) multiple times
			// since event_cancel resets only the caller's pointer
			if (the_event->q_el == pElem) {
				the_event->q_el = nullptr;
			}
			continue;
		}

		new_time = pElem->iKey;

 

Which makes total sense, i have no idea why op setted q_el to null after free on the other 2 spots but not there, @ Kuqsal you should totally update the post.

@ Intel your scenario is the reason why this is a common practice:

51 minutes ago, Froslass said:

Common programming practices on the other hand say that is always better to set a pointer to NULL rather than having a dangling pointer even if we are sure nothing will access that pointer while dangling, because if somebody change some part of the code related to it in the future it will cause a crash

Very interesting, thanks for sharing