Originally posted by [b]theultramage[/b]
http://www.eathena.ws/board/index.php?autocom=bugtracker&showbug=4007
In status_change_timer, there are several statuses that cause damage and can kill its subject.
In particular, SC_BLEEDING and SC_POISON/DPOISON.
Their layout looks like this:
CODE
int flag;
map_freeblock_lock();
status_fix_damage(NULL, bl, rand()%600 + 200, 0);
flag = !sc->data[type];
map_freeblock_unlock();
if (flag) return 0; //SC already ended.
sc_timer_next(10000 + tick, status_change_timer, bl->id, data);
return 0;
First block list freeing is disabled, then damage is done (which kills the mob). Then before unlocking, the flag is remembered.
Then the freeblock cache is unlocked and the mob's memory is freed. The sc->data[type] was invalidated, that's why the code remembered the value earlier. If the status ended (most statuses end on death), no more processing is done and the function exits.
Now what happens if the status for some reason remains?
Other than accessing bl->id, when bl has already been trashed beyond recognition, the sc_timer_next() macro is ran:
CODE
#define sc_timer_next(t,f,i,d) \
if( (sce=sc->data[type]) ) \
sce->timer = add_timer(t,f,i,d); \
else \
ShowError("status_change_timer: Unexpected NULL status change id: %d data: %d\n", id, data)
This causes access to sc->data[]->timer, where sc->data is garbage - thus resulting in a random memory read, and the followup SIGSEGV.
What makes me wonder is, if the code couldn't have been written this way...
CODE
map_freeblock_lock();
status_fix_damage(NULL, bl, rand()%600 + 200, 0);
if (sc->data[type]) //SC still active
sc_timer_next(10000 + tick, status_change_timer, bl->id, data);
map_freeblock_unlock();
return 0;
That is, no flag variable crap, no premature unlocking, no multiple exit points.
Oh, and sometime earlier I suggested wrapping the entire core loop in a lock/unlock block, to avoid crashes like this. Of course, the best way would be to make sure the code doesn't need to do any freeing until the very end. Even though the data is not freed yet, it is unit_free()'d, which means it's been stripped of pretty much everything, and this can cause erratic behavior.