Issue information

Issue ID
#3253
Status
Fixed
Severity
Medium
Started
Hercules Elf Bot
Jun 15, 2009 14:09
Last Post
Hercules Elf Bot
Jun 15, 2009 14:09
Confirmation
N/A

Hercules Elf Bot - Jun 15, 2009 14:09

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

Each time a land skill is created, a secondary pointer to it gets recorded in its owner's ud->skillunit[] array. This allows quick processing of maxcount checks (pneuma can only be placed 3 times), mass clearing (when leaving map), and several other functions whose purpose is unknown (skill_clear_group, skill_locate_element_field). When a land skill expires, it's the job of skill_delunitgroup() to erase any reference to it. It does so by retrieving its owner's skillunit[] array and processing it.

Until r5991, skill_delunitgroup() obtained the 'owner' by looking up its group->src_id. Since this revision, the function has an optional 'src' parameter, which will be used as the 'owner' if specified. Skotlex told me its purpose was to skip the lookup step. However, since skill_delunitgroup must process the group owner's skillunit array, it absolutely has to execute with src == map_id2bl(group->src_id)!

If it runs with someone else as 'src', then three things happen:
1. the group isn't found in src's array, and nothing is done.
2. the group data is erased.
2. the pointer to the group still remains unchanged in the real owner's array.

Once the server reaches such a state, then at the moment the real owner logs out, this code executes:
CODE
    while (ud->skillunit[0])
        skill_delunitgroup(src, ud->skillunit[0]);
and once it encounters the dangling reference, it will write to memory that's been deallocated long ago, producing a segfault and/or memory corruption.

Did not test yet, however this is what I predict will happen. My suggestion is to remove the extra 'src' parameter, and perhaps also consider removing the skillunit[] array.