Issue information

Issue ID
#4982
Status
Fixed
Severity
Critical
Started
Hercules Elf Bot
Jun 26, 2011 16:26
Last Post
Hercules Elf Bot
Jun 26, 2011 16:26
Confirmation
N/A

Hercules Elf Bot - Jun 26, 2011 16:26

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

There is a copy-paste typo in function quest_add() that trashes the in-memory quest index of all already finished quests. If the index is used without re-logging, it will cause an out of bounds read, possibly resulting in a server crash.
QUOTE
memmove(&sd->quest_log[i+1], &sd->quest_log[i], sizeof(struct quest)*(sd->num_quests-sd->avail_quests));
memmove(sd->quest_index+i+1, sd->quest_log+i, sizeof(int)*(sd->num_quests-sd->avail_quests));

The questlog array is split into two parts - active quests, followed by completed quests. When adding a new quest, all completed quests are shifted one array position to the right using memmove(). The same thing is then done to the quest_index array, which is paired with the questlog array.

The typo was introduced 2 years ago in r14039 as a followup to r14038. The reason why it wasn't discovered earlier is, that the index corruption only affects already completed entries, and that the index is actually used only in 2 places, one of which only processes active entries. And even the remaining one only executes vulnerable code as a special case.
CODE
int quest_check(TBL_PC * sd, int quest_id, quest_check_type type)
{...
    case HUNTING:
    ARR_FIND(0, MAX_QUEST_OBJECTIVES, j, sd->quest_log[i].count[j] < quest_db[sd->quest_index[i]].count[j]);

To actually execute this, you need to 1) start a hunting quest, 2) have it marked as completed, but not delete it, 3) start another quest without relogging, 4) execute checkquest(HUNTING) on the already completed quest id. These are very special circumstances - however, that's exactly what the renewal novice ground script does.

PS: here's my reproduction script (might not actually crash, I used a debugger to verify the issue).
CODE
prontera.gat,145,175,4    script    Test Poring    909,{

    // cleanup
    erasequest 1001;
    erasequest 60107; // "Hunting Fabres"

    // setup
    setquest 60107;
    completequest 60107;

    // corrupt
    setquest 1001;

     // crash
    checkquest(60107,HUNTING);
}


PS: Crash was reported by Gepard in #athena.

This post has been edited by theultramage: Jun 26 2011, 10:02 AM