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

Replace OOB guards with safe RNG calls when initialising quests #7013

Merged
merged 1 commit into from
Mar 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 14 additions & 41 deletions Source/quests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -104,30 +104,6 @@ const char *const QuestTriggerNames[5] = {
N_(/* TRANSLATORS: Quest Map*/ "A Dark Passage"),
N_(/* TRANSLATORS: Quest Map*/ "Unholy Altar")
};
/**
* A quest group containing the three quests the Butcher,
* Ogden's Sign and Gharbad the Weak, which ensures that exactly
* two of these three quests appear in any single player game.
*/
int QuestGroup1[3] = { Q_BUTCHER, Q_LTBANNER, Q_GARBUD };
/**
* A quest group containing the three quests Halls of the Blind,
* the Magic Rock and Valor, which ensures that exactly two of
* these three quests appear in any single player game.
*/
int QuestGroup2[3] = { Q_BLIND, Q_ROCK, Q_BLOOD };
/**
* A quest group containing the three quests Black Mushroom,
* Zhar the Mad and Anvil of Fury, which ensures that exactly
* two of these three quests appear in any single player game.
*/
int QuestGroup3[3] = { Q_MUSHROOM, Q_ZHAR, Q_ANVIL };
/**
* A quest group containing the two quests Lachdanan and Warlord
* of Blood, which ensures that exactly one of these two quests
* appears in any single player game.
*/
int QuestGroup4[2] = { Q_VEIL, Q_WARLORD };

/**
* @brief There is no reason to run this, the room has already had a proper sector assigned
Expand Down Expand Up @@ -274,7 +250,7 @@ void InitQuests()
}

if (!UseMultiplayerQuests() && *sgOptions.Gameplay.randomizeQuests) {
// Quests are set from the seed used to generate level 16.
// Quests are set from the seed used to generate level 15.
InitialiseQuestPools(DungeonSeeds[15], Quests);
}

Expand All @@ -301,25 +277,22 @@ void InitialiseQuestPools(uint32_t seed, Quest quests[])
DiabloGenerator rng(seed);
quests[rng.pickRandomlyAmong({ Q_SKELKING, Q_PWATER })]._qactive = QUEST_NOTAVAIL;

// using int and not size_t here to detect negative values from GenerateRnd
int randomIndex = rng.generateRnd(sizeof(QuestGroup1) / sizeof(*QuestGroup1));

if (randomIndex >= 0)
quests[QuestGroup1[randomIndex]]._qactive = QUEST_NOTAVAIL;

randomIndex = rng.generateRnd(sizeof(QuestGroup2) / sizeof(*QuestGroup2));
if (randomIndex >= 0)
quests[QuestGroup2[randomIndex]]._qactive = QUEST_NOTAVAIL;
if (seed == 988045466) {
// If someone starts a new game at 1977-12-28 19:44:42 UTC or 2087-02-18 22:43:02 UTC
// vanilla Diablo ends up reading QuestGroup1[-2] here. Due to the way the data segment
// is laid out (at least in 1.09) this ends up reading the address of the string
// "A Dark Passage" and trying to write to Quests[<addr>*8] which lands in read-only memory.
// The proper result would've been to mark The Butcher unavailable but instead nothing happens.
rng.discardRandomValues(1);
} else {
quests[rng.pickRandomlyAmong({ Q_BUTCHER, Q_LTBANNER, Q_GARBUD })]._qactive = QUEST_NOTAVAIL;
}

randomIndex = rng.generateRnd(sizeof(QuestGroup3) / sizeof(*QuestGroup3));
if (randomIndex >= 0)
quests[QuestGroup3[randomIndex]]._qactive = QUEST_NOTAVAIL;
quests[rng.pickRandomlyAmong({ Q_BLIND, Q_ROCK, Q_BLOOD })]._qactive = QUEST_NOTAVAIL;

randomIndex = rng.generateRnd(sizeof(QuestGroup4) / sizeof(*QuestGroup4));
quests[rng.pickRandomlyAmong({ Q_MUSHROOM, Q_ZHAR, Q_ANVIL })]._qactive = QUEST_NOTAVAIL;

// always true, QuestGroup4 has two members
if (randomIndex >= 0)
quests[QuestGroup4[randomIndex]]._qactive = QUEST_NOTAVAIL;
quests[rng.pickRandomlyAmong({ Q_VEIL, Q_WARLORD })]._qactive = QUEST_NOTAVAIL;
}

void CheckQuests()
Expand Down
19 changes: 17 additions & 2 deletions test/quests_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,31 @@ TEST(QuestTest, SinglePlayerBadPools)
EXPECT_EQ(Quests[Q_SKELKING]._qactive, QUEST_NOTAVAIL) << "Skeleton King quest is deactivated with 'bad' seed";
ResetQuests();

// Having this seed for level 15 requires starting a game at 1977-12-28 07:44:42 PM or 2087-02-18 10:43:02 PM
// Given Diablo was released in 1996 and it's currently 2024 this will never have been naturally hit. It's not
// possible to hit this case on a pre-NT kernel windows system but it may be possible on macos or winnt?
InitialiseQuestPools(988045466, Quests);
EXPECT_THAT(GetActiveFlagsForSlice({ Q_BUTCHER, Q_LTBANNER, Q_GARBUD }), ::testing::Each(QUEST_INIT)) << "All quests in pool 2 remain active with 'bad' seed";
ResetQuests();

// This seed can only be reached by editing a save file or modifying the game. Given quest state (including
// availability) is saved as part of the game state there's no vanilla compatibility concerns here.
InitialiseQuestPools(4203210069U, Quests);
EXPECT_THAT(GetActiveFlagsForSlice({ Q_BLIND, Q_ROCK, Q_BLOOD }), ::testing::Each(QUEST_INIT)) << "All quests in pool 3 remain active with 'bad' seed";
// If we wanted to retain the behaviour that vanilla Diablo would've done we should instead deactivate
// Quests[QuestGroup2[-2]]. This would hit QuestGroup1[1] (Ogden's Sign), however that's already marked
// as unavailable with this seed due to the previous quest group's roll.
EXPECT_EQ(Quests[Q_LTBANNER]._qactive, QUEST_NOTAVAIL) << "Ogden's Sign should be deactivated with 'bad' seed";
EXPECT_EQ(Quests[Q_BLIND]._qactive, QUEST_NOTAVAIL) << "Halls of the Blind should also be deactivated with 'bad' seed";
ResetQuests();

// This seed can only be reached by editing a save file or modifying the game. Given quest state (including
// availability) is saved as part of the game state there's no vanilla compatibility concerns here.
InitialiseQuestPools(2557708932U, Quests);
EXPECT_THAT(GetActiveFlagsForSlice({ Q_MUSHROOM, Q_ZHAR, Q_ANVIL }), ::testing::Each(QUEST_INIT)) << "All quests in pool 4 remain active with 'bad' seed";
// If we wanted to retain the behaviour that vanilla Diablo would've done we should instead deactivate
// Quests[QuestGroup3[-2]]. This would hit QuestGroup2[1] (Magic Rock), however that's already marked
// as unavailable with this seed due to the previous quest group's roll.
EXPECT_EQ(Quests[Q_ROCK]._qactive, QUEST_NOTAVAIL) << "Magic Rock should be deactivated with 'bad' seed";
EXPECT_EQ(Quests[Q_MUSHROOM]._qactive, QUEST_NOTAVAIL) << "Black Mushroom should also be deactivated with 'bad' seed";
ResetQuests();

InitialiseQuestPools(1272442071, Quests);
Expand Down
Loading