clone killing itself with PA_SACRIFICE crash - Hercules bug tracker archive

Issue information

Issue ID

#4950

Status

Fixed

Severity

None

Started

Hercules Elf Bot

Jun 6, 2011 13:40

Last Post

Hercules Elf Bot

Apr 18, 2012 15:38

Confirmation

N/A

Hercules Elf Bot - Jun 6, 2011 13:40

Originally posted by [b]theultramage[/b]
http://www.eathena.ws/board/index.php?autocom=bugtracker&showbug=4950

in battle_weapon_attack():

CODE

        if (sc->data[SC_SACRIFICE])
        {
            ...
            status_zap(src, sstatus->max_hp*9/100, 0);//Damage to self is always 9%
            return (damage_lv)skill_attack(BF_WEAPON,src,src,target,PA_SACRIFICE,skilllv,tick,0);

Calling status_zap() can kill the user, in which case skill_attack() will execute with a dead unit.

Now consider what happens when a player clone does this. Upon death, it is immediately unit_free'd, which involves calling

CODE

int mob_clone_delete(struct mob_data *md)
{
    const int class_ = md->class_;
    if (class_ >= MOB_CLONE_START && class_ < MOB_CLONE_END && mob_db_data[class_]!=NULL) {
        aFree(mob_db_data[class_]);
        mob_db_data[class_]=NULL;
        //Clear references to the db
        md->db = mob_dummy;
        md->vd = NULL;}

So not only does the clone have partially removed data, it also no longer has its mobdb entry, and more importantly, its view data is set to NULL. This ultimately leads to a crash in status_get_class(), which expects a non-null md->vd (crash is in battle_calc_weapon_attack).

To reproduce, just make a strong enough paladin with martyr's reckoning, and damage its health by at least 10% (then clones start casting self skills).

Hercules Elf Bot - Dec 25, 2011 17:59

Originally posted by [b]xazax[/b]
What about calculating skill_attack first, storing it's return value. Call status_zap after that, and return the stored dmg.

Hercules Elf Bot - Dec 25, 2011 19:47

Originally posted by [b]Ind[/b]
[quote name='xazax' timestamp='1324835995' post='6021']
What about calculating skill_attack first, storing it's return value. Call status_zap after that, and return the stored dmg.
[/quote]
but would it be worth the effort? rewriting how the whole thing works prior to fixing this custom-feature-made bug, what if we delayed it's 'free'? the mob_dead already delays "client death" of clones, what if we added a timer to release it? something like 0.5s would be more than enough for the rest of the calc proceed in order for it to not crash

This post has been edited by Ind on Dec 25, 2011 19:48

Hercules Elf Bot - Dec 25, 2011 20:03

Originally posted by [b]xazax[/b]
I fix it my way, because it involves less serious modifications. However if we find similar crash bugs, than your solution should be applied since it would fix all of them globally. Fixed in [rev=15277]

This post has been edited by xazax on Dec 25, 2011 20:07