Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

items: add BUGFIX for SpawnWitch #2255

Closed
wants to merge 1 commit into from

Conversation

mewmew
Copy link
Contributor

@mewmew mewmew commented Sep 22, 2021

The logic handling the list of items for sale by Adria is broken. In
particular, the logic of SortWitch requires the list to end with an
item having item type ITYPE_NONE.

From SortWitch:

	j = 3;
	while (witchitem[j + 1]._itype != ITYPE_NONE) {
		j++;
	}

However, this puts a limit (19) on the maximum number of items being
generated for the witchitem array (of length 20). The problem is that
SpawnWitch may generate 20 items if iCnt = 17, as made possible by
the following statement.

	iCnt = random_(51, 8) + 10;

Since there are 3 items that are always generated (mana, full mana
and town portal), the total number of items generated by SpawnWitch
may be 20, which exceeds the limit (19), thus causing the logic of
SortWitch to go haywire and results in a buffer overflow.

The logic handling the list of items for sale by Adria is broken. In
particular, the logic of SortWitch requires the list to end with an
item having item type ITYPE_NONE.

From SortWitch:
	j = 3;
	while (witchitem[j + 1]._itype != ITYPE_NONE) {
		j++;
	}

However, this puts a limit (19) on the maximum number of items being
generated for the witchitem array (of length 20). The problem is that
SpawnWitch may generate 20 items if `iCnt = 17`, as made possible by
the following statement.

	iCnt = random_(51, 8) + 10;

Since there are 3 items that are always generated (mana, full mana
and town portal), the total number of items generated by SpawnWitch
may be 20, which exceeds the limit (19), thus causing the logic of
SortWitch to go haywire and results in a buffer overflow.
@mewmew
Copy link
Contributor Author

mewmew commented Sep 23, 2021

Same issue in SpawnHealer in Multi Player games, causing buffer overflow in SortHealer.

@AJenbo
Copy link
Member

AJenbo commented Sep 23, 2021

Isn't that an issue with SortWitch?

@galaxyhaxz
Copy link
Member

galaxyhaxz commented Sep 23, 2021

I don't think it causes an overflow like the bug you mentioned, but there is an additional bug with SortHealer. It begins at item index 2, which is correct for single games. However in multi he also has resurrect scrolls, so the index should begin at 3 there.

	if (gbMaxPlayers != 1) {
		GetItemAttrs(0, IDI_RESURRECT, 1);
		healitem[2] = item[0];
		healitem[2]._iCreateInfo = lvl;
		healitem[2]._iStatFlag = TRUE;
		srnd = 3;
	} else srnd = 2;

Edit: Since the hardcoded resurrect scrolls come before the random item indices, it will stay 3rd on the list. however if one were to move it down on the list, it could get mixed up with other items.

@galaxyhaxz
Copy link
Member

galaxyhaxz commented Sep 23, 2021

I took a closer look at this, I don't think it's actually bugged in this way, but rather another. If you look here:

	nsi = random(8) + 10;
	for (i = 3; i < nsi; i++) {

The random index is 10-17. This means at most we would be scanning indices 3-17 of the witch item array, leaving the last 3 untouched. This is because this is bugged and should be:

for (i = 3; i < nsi + 3; i++)

In order to actually touch all 20 indices. Of course, as mentioned, if this bug were fixed it would cause the overflow so you'd want to cap it to 19 or fix it another way.

EDIT: same bug applies to healer, he starts at index 2 or 3 and only goes to at most 17, leaving the last two slots untouched.

@mewmew mewmew closed this Oct 14, 2022
@mewmew
Copy link
Contributor Author

mewmew commented Oct 14, 2022

Closed this PR, as the bug had another cause, as detailed by Andi in #2255 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants