BUG: Undefined Behavior due to Achievement Propagation

[dawnreprime]

Issue Description

Due to the way the achievement system is coded there exists a possibility that undefined behavior modifies the tracking incorrectly and adds additional progress to future meta achievements (probably the best-case scenario) when completing the one before it.

This is due to

DarkflameServer/dGame/dComponents/MissionComponent.cpp

Line 144 in a713216

void MissionComponent::Progress(eMissionTaskType type, int32_t value, LWOOBJID associate, const std::string& targets, int32_t count, bool ignoreAchievements) {

being recursive in a depth-first manner alongside the fact that

DarkflameServer/dGame/dComponents/MissionComponent.h

Line 180 in a713216

std::unordered_map<uint32_t, Mission*> m_Missions;

is being modified whilst being iterated over during the tree execution.

This (can) produce an execution as follows:

1. mission A completes meta mission M
2. meta mission N gets 1 from meta mission M's completion
3. meta mission N gets 1 from mission A's completion

leading to the extra mission contributing to a tier it should not.

In C++, it’s typically not a great idea to mutate over certain collections having iterator invalidation without taking precautions. Even if being careful about unordered_map’s specific cases about never calling erase() (specific element invalidated) and even adding a band-aid reserve() to avoid rehashes on insert() (complete iterator invalidation). Also, the behavior can differ depending on standard library implementations, binaries, and even different runs.

Reproduction Steps

• If you are blessed enough to test your software on a single OS (GCC + Linux/Docker), the bug can be seen happening when unlocking the final tier of meta achievements.

• If you are running on the MSVC Windows version, you’ll find that it happens on every subsequent meta achievement.

I really wish Clang Windows worked with this project… but compilers don’t fix UB!

Expected Behavior

Upon progressing to the next meta achievement, the value should start at 1/N instead of 2/N (Ensure to not trigger multiple achievements on the last step).

Environment

• GCC Linux/Docker, MSVC Windows

• SQLite or MariaDB

Workaround

This is what I came up with to stop UB happening during the recursive tree execution:

#include <utility>

DarkflameServer/dGame/dComponents/MissionComponent.cpp

Lines 144 to 150 in a713216

	void MissionComponent::Progress(eMissionTaskType type, int32_t value, LWOOBJID associate, const std::string& targets, int32_t count, bool ignoreAchievements) {
	LOG("Progressing missions %s %i %llu %s %s", StringifiedEnum::ToString(type).data(), value, associate, targets.c_str(), ignoreAchievements ? "(ignoring achievements)" : "");
	std::vector<uint32_t> acceptedAchievements;
	if (count > 0 && !ignoreAchievements) {
	acceptedAchievements = LookForAchievements(type, value, true, associate, targets, count);
	}

	std::vector<Mission*> missions;
	missions.reserve(m_Missions.size());
	std::transform(m_Missions.begin(), m_Missions.end(), std::back_inserter(missions), [](const std::pair<uint32_t, Mission*>& pair) { return pair.second; });

	for (auto* mission : missions) {

I realize this is not too performant given its execution location (tens of times among many players) and the fact that usually just a single value is inserted to the m_Missions unordered_map potentially, but due to the explanation earlier this could potentially lead to a rehash and possibly a larger problem in the future with code modifications. It would be better if a developer more experienced with the project’s source refactored these sections to no longer depend on UB.

[dawnreprime]

Suggested Labels: [A-game, B-components, B-mission, bug, help wanted, performance, S-fixed]

[dawnreprime]

I drafted another fix that relies on far less duplication of data across all these calls. Once I get access to my dev tools I will upload a PR with that fix and this issue can possibly be used for further optimization or refactoring of these sections.