Skip to content
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
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,46 @@
#include "OloEngine/Scene/Components.h"
#include "OloEngine/Serialization/Archive.h"

#include <cmath>

namespace OloEngine
{
namespace
{
// Snapshot bytes arrive from the network, which is untrusted: a NaN / ±inf
// float poisons transform math (and, downstream, integer tick casts —
// see SnapshotInterpolator). Replace any non-finite wire float with a safe
// fallback. See docs/agent-rules/cpp-coding-quality.md §2.
[[nodiscard]] f32 SanitizeWireFloat(f32 value, f32 fallback)

Check warning on line 16 in OloEngine/src/OloEngine/Networking/Replication/ComponentReplicator.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Add message to "[[nodiscard]]" attribute explaining why the return value should not be discarded.

See more on https://sonarcloud.io/project/issues?id=drsnuggles8_OloEngineBase&issues=AZ8APdho901sLPN1VVbA&open=AZ8APdho901sLPN1VVbA&pullRequest=400
{
return std::isfinite(value) ? value : fallback;
}

[[nodiscard]] glm::vec3 SanitizeWireVec3(const glm::vec3& value, const glm::vec3& fallback)

Check warning on line 21 in OloEngine/src/OloEngine/Networking/Replication/ComponentReplicator.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Add message to "[[nodiscard]]" attribute explaining why the return value should not be discarded.

See more on https://sonarcloud.io/project/issues?id=drsnuggles8_OloEngineBase&issues=AZ8APdho901sLPN1VVbB&open=AZ8APdho901sLPN1VVbB&pullRequest=400
{
return {
SanitizeWireFloat(value.x, fallback.x),
SanitizeWireFloat(value.y, fallback.y),
SanitizeWireFloat(value.z, fallback.z),
};
}
} // namespace

void ComponentReplicator::Serialize(FArchive& ar, TransformComponent& component)
{
OLO_PROFILE_FUNCTION();

ar << component.Translation.x << component.Translation.y << component.Translation.z;
glm::vec3 euler = component.GetRotationEuler();
ar << euler.x << euler.y << euler.z;
ar << component.Scale.x << component.Scale.y << component.Scale.z;
if (ar.IsLoading())
{
component.SetRotationEuler(euler);
// Validate every float pulled off the wire before it reaches the scene.
component.Translation = SanitizeWireVec3(component.Translation, glm::vec3(0.0f));
component.Scale = SanitizeWireVec3(component.Scale, glm::vec3(1.0f));
component.SetRotationEuler(SanitizeWireVec3(euler, glm::vec3(0.0f)));
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
}
ar << component.Scale.x << component.Scale.y << component.Scale.z;
}

void ComponentReplicator::Serialize(FArchive& ar, Rigidbody2DComponent& component)
Expand Down Expand Up @@ -45,6 +71,18 @@
ar << component.m_Mass;
ar << component.m_InitialLinearVelocity.x << component.m_InitialLinearVelocity.y << component.m_InitialLinearVelocity.z;
ar << component.m_InitialAngularVelocity.x << component.m_InitialAngularVelocity.y << component.m_InitialAngularVelocity.z;
if (ar.IsLoading())
{
// Untrusted wire floats: mass must be finite and strictly positive; velocities finite.
// Zero is rejected too — a dynamic body with zero mass divides by zero in the Jolt
// mass-properties override (JoltBody.cpp), so default it to the safe positive minimum.
if (!std::isfinite(component.m_Mass) || component.m_Mass <= 0.0f)
{
component.m_Mass = 1.0f;
}
component.m_InitialLinearVelocity = SanitizeWireVec3(component.m_InitialLinearVelocity, glm::vec3(0.0f));
component.m_InitialAngularVelocity = SanitizeWireVec3(component.m_InitialAngularVelocity, glm::vec3(0.0f));
}
}

std::unordered_map<std::string, ComponentSerializeFn> ComponentReplicator::s_Registry;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "OloEngine/Serialization/Archive.h"

#include <algorithm>
#include <cmath>
#include <unordered_map>

#include <glm/gtc/quaternion.hpp>
Expand Down Expand Up @@ -68,7 +69,11 @@ namespace OloEngine
f32 const delayTicks = m_RenderDelay * static_cast<f32>(m_ServerTickRate);
f32 const renderTick = static_cast<f32>(m_LatestReceivedTick) - delayTicks;

if (renderTick < 0.0f)
// Defend the integer cast below: a non-finite renderTick (which a NaN slips
// past `< 0.0f`, since every comparison with NaN is false) would make the
// static_cast<u32> undefined behaviour. m_RenderDelay is validated at the
// setter, but guard here too — wire/config data is untrusted.
if (!std::isfinite(renderTick) || renderTick < 0.0f)
{
return;
}
Expand Down Expand Up @@ -165,6 +170,15 @@ namespace OloEngine

void SnapshotInterpolator::SetRenderDelay(f32 seconds)
{
// Render delay feeds the tick math in Interpolate()/GetRenderTick(); a
// non-finite or negative value would poison renderTick and the u32 cast
// (NaN slips past `< 0.0f`). Reject it and keep the previous valid delay.
if (!std::isfinite(seconds) || seconds < 0.0f)
{
OLO_CORE_WARN("[SnapshotInterpolator] Ignoring invalid render delay {} (must be finite and >= 0); keeping {}", seconds, m_RenderDelay);
return;
}

m_RenderDelay = seconds;
}

Expand Down
48 changes: 45 additions & 3 deletions OloEngine/src/OloEngine/Task/Scheduler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@
#include "OloEngine/Debug/Instrumentor.h"

#include <algorithm>
#include <cmath>
#include <cstdlib>
#include <cstring>
#include <optional>

// Platform cache line size (typically 64 bytes on x86/x64)
#ifndef PLATFORM_CACHE_LINE_SIZE
Expand All @@ -42,6 +44,41 @@
static bool g_TaskGraphUseDynamicThreadCreation = false;
static bool g_TaskGraphConfigInitialized = false;

std::optional<f32> ParseOversubscriptionRatio(const char* envValue)

Check warning on line 47 in OloEngine/src/OloEngine/Task/Scheduler.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

This function has 5 returns, which is more than the 3 allowed.

See more on https://sonarcloud.io/project/issues?id=drsnuggles8_OloEngineBase&issues=AZ8DxnUQKqvs92CVe-zU&open=AZ8DxnUQKqvs92CVe-zU&pullRequest=400
{
if (envValue == nullptr)
{
return std::nullopt;
}

// Parse with std::strtof rather than std::atof: atof has no error reporting and
// undefined behaviour on overflow, whereas strtof reports "not a number at all"
// via endPtr and overflow via ±HUGE_VALF (== ±inf). "inf"/"nan" likewise parse to
// non-finite values. All of those must be rejected before they reach the
// worker-budget math (inf >= 1.0f is true; the product ceil(workers * inf) then
// casts to i32, which is undefined behaviour) — the endPtr and isfinite guards
// below do exactly that.
char* endPtr = nullptr;
const f32 value = std::strtof(envValue, &endPtr);
if (endPtr == envValue)
{
// No characters consumed → the value is not a number.
return std::nullopt;
}
if (*endPtr != '\0')
{
// Trailing, unconsumed characters → a partially-numeric string like "2.0abc".
// strtof would silently parse the "2.0" prefix; require the whole string instead.
return std::nullopt;
}
if (!std::isfinite(value) || value < 1.0f || value > kMaxOversubscriptionRatio)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
{
return std::nullopt;
}

return value;
}

// @brief Parse command-line or environment configuration for task graph settings
//
// This mirrors UE5.7's approach of parsing command-line arguments like:
Expand Down Expand Up @@ -96,10 +133,15 @@

if (const char* EnvOversubscriptionRatio = std::getenv("OLO_TASK_GRAPH_OVERSUBSCRIPTION_RATIO"))
{
float Value = static_cast<float>(std::atof(EnvOversubscriptionRatio));
if (Value >= 1.0f)
if (const std::optional<f32> Ratio = ParseOversubscriptionRatio(EnvOversubscriptionRatio))
{
g_TaskGraphOversubscriptionRatio = *Ratio;
}
else
{
g_TaskGraphOversubscriptionRatio = Value;
OLO_CORE_WARN("[TaskGraph] Ignoring invalid OLO_TASK_GRAPH_OVERSUBSCRIPTION_RATIO='{}' "
"(must be finite and within [1, {}]); keeping {}",
EnvOversubscriptionRatio, kMaxOversubscriptionRatio, g_TaskGraphOversubscriptionRatio);
}
}
}
Expand Down
14 changes: 14 additions & 0 deletions OloEngine/src/OloEngine/Task/Scheduler.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,23 @@

#include <atomic>
#include <memory>
#include <optional>

namespace OloEngine::LowLevelTasks
{
// Upper bound accepted for the task-graph oversubscription ratio. The ratio
// multiplies the worker-thread count, so an unbounded (or non-finite) value
// read from config would blow up the thread budget — clamp it to something sane.
inline constexpr f32 kMaxOversubscriptionRatio = 64.0f;

// Parse and validate the OLO_TASK_GRAPH_OVERSUBSCRIPTION_RATIO environment value.
// Environment config is untrusted input: a non-finite (NaN / ±inf) or out-of-range
// value must be rejected before it reaches the scheduler's worker-budget math
// (std::strtof yields inf for "inf", and `inf >= 1.0f` is true). Returns the
// validated ratio, or std::nullopt when the value is null/unparsable/out of the
// accepted [1, kMaxOversubscriptionRatio] range. Pure + side-effect free for testing.
[[nodiscard]] std::optional<f32> ParseOversubscriptionRatio(const char* envValue);

Check failure on line 36 in OloEngine/src/OloEngine/Task/Scheduler.h

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Do not pass an array as a single pointer. Consider using std::string_view to avoid array to pointer decay.

See more on https://sonarcloud.io/project/issues?id=drsnuggles8_OloEngineBase&issues=AZ8APdlD901sLPN1VVbD&open=AZ8APdlD901sLPN1VVbD&pullRequest=400

Check warning on line 36 in OloEngine/src/OloEngine/Task/Scheduler.h

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Add message to "[[nodiscard]]" attribute explaining why the return value should not be discarded.

See more on https://sonarcloud.io/project/issues?id=drsnuggles8_OloEngineBase&issues=AZ8APdlD901sLPN1VVbC&open=AZ8APdlD901sLPN1VVbC&pullRequest=400

// @enum EQueuePreference
// @brief Preference for which queue to use when launching tasks
enum class EQueuePreference
Expand Down
2 changes: 2 additions & 0 deletions OloEngine/tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,8 @@ add_executable(OloEngine-Tests
Networking/ChatTest.cpp
Networking/PersistenceTest.cpp
Networking/MMOOptimizationTest.cpp
# Task / Scheduler config tests
Task/SchedulerConfigTest.cpp
# Localization tests
LocalizationTest.cpp
# Dialogue system tests
Expand Down
Loading
Loading