Skip to content

fix(net,task): isfinite-guard untrusted float ingestion (network snap… - #400

Merged
drsnuggles8 merged 3 commits into
masterfrom
feature/net-task-untrusted-float-hardening
Jun 26, 2026
Merged

drsnuggles8 merged 3 commits into
masterfrom
feature/net-task-untrusted-float-hardening

Conversation

@drsnuggles8

@drsnuggles8 drsnuggles8 commented Jun 25, 2026 •

Copy link
Copy Markdown
Owner

…shots + scheduler env config)

Summary by CodeRabbit

  • Bug Fixes
    • Hardened component replication/deserialization and snapshot timing against non-finite floats (NaN/±inf), preventing invalid values from contaminating scene/physics math.
    • Sanitized transform and physics parameters (with safe fallbacks) and enforced stricter mass and velocity validation.
    • Task scheduler now validates oversubscription environment input, ignoring malformed/out-of-range/non-finite values while retaining the last valid setting.
  • Tests
    • Added/extended unit coverage for invalid float handling in component replication, snapshot interpolation, and scheduler configuration.
  • Documentation
    • Expanded coding-quality guidance to emphasize validating untrusted floats at network and configuration boundaries.

@coderabbitai

coderabbitai Bot commented Jun 25, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 172c338a-0a52-4947-aacc-6777695e2593

📥 Commits

Reviewing files that changed from the base of the PR and between b942a2c and 0fd26fe.

📒 Files selected for processing (4)
  • OloEngine/src/OloEngine/Networking/Replication/ComponentReplicator.cpp
  • OloEngine/src/OloEngine/Task/Scheduler.cpp
  • OloEngine/tests/Networking/ComponentReplicatorTest.cpp
  • OloEngine/tests/Task/SchedulerConfigTest.cpp

📝 Walkthrough

Walkthrough

The PR adds finite-value validation for replicated component floats, snapshot interpolation timing inputs, and task-graph oversubscription parsing. It also adds tests for the new checks and updates coding guidance for untrusted float boundaries.

Changes

Untrusted float boundary hardening

Layer / File(s) Summary
Replication sanitization
OloEngine/src/OloEngine/Networking/Replication/ComponentReplicator.cpp, OloEngine/tests/Networking/ComponentReplicatorTest.cpp
Adds wire-float sanitizers and applies them while loading transform and rigidbody components from replicated data, with tests covering NaN, infinities, and negative mass.
Render-delay validation
OloEngine/src/OloEngine/Networking/Replication/SnapshotInterpolator.cpp, OloEngine/tests/Networking/SnapshotInterpolatorTest.cpp
Rejects non-finite render ticks and render-delay inputs, preserves the previous delay on invalid values, and adds tests for rejection and accepted zero delay.
Oversubscription parsing
OloEngine/src/OloEngine/Task/Scheduler.h, OloEngine/src/OloEngine/Task/Scheduler.cpp, OloEngine/tests/CMakeLists.txt, OloEngine/tests/Task/SchedulerConfigTest.cpp, docs/agent-rules/cpp-coding-quality.md
Declares and implements a bounded parser for environment oversubscription ratios, uses it during task-graph configuration, registers the new test file, adds parser tests, and updates the float-validation guidance.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main changes: hardening untrusted float ingestion in networking and task scheduler paths with isfinite validation.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

… S989)

std::atof has UB on overflow and no error reporting; SonarCloud rule
cpp:S989 flags it CRITICAL and failed the PR's D-Reliability quality gate.
Parse with std::strtof instead: reject not-a-number via endPtr, keep the
isfinite/range guard (overflow returns +-HUGE_VALF, caught by isfinite).
strtof is not flagged by S989 and, unlike float std::from_chars, is
available on the Linux/Clang(libc++) CI.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@drsnuggles8

Copy link
Copy Markdown
Owner Author

🤖 Self-review (finish-pr) @ b942a2c0f9c49648b7ff43ab7a09323c9d01cb24

Reviewed the full PR diff against master at high effort.

  • Findings: 1 · Fixed: replaced std::atof with std::strtof in ParseOversubscriptionRatio (Scheduler.cpp) — atof has UB on overflow and no error reporting (SonarCloud cpp:S989, CRITICAL, the cause of the failing D Reliability on New Code quality gate). strtof rejects not-a-number via endPtr, and overflow (±HUGE_VALF) is still caught by the existing isfinite/range guard. Chosen over float std::from_chars because the latter isn't available on the Linux/Clang(libc++) CI.
  • Dismissed: none.
  • Coverage check: hardening is complete across all three registered ComponentReplicator serializers — Rigidbody2DComponent serializes only bodyType+FixedRotation (no wire floats), so it correctly needs no guard.
  • Verification: built OloEngine-Tests (Debug, exit 0); 29/29 pass across SchedulerConfigTest/ComponentReplicatorTest/SnapshotInterpolatorTest. Visual evidence N/A — pure network/config/CPU change, no rendering surface touched.

@drsnuggles8 drsnuggles8 added the self-reviewed finish-pr self-review done label Jun 26, 2026
coderabbitai[bot]

This comment was marked as outdated.

@drsnuggles8

Copy link
Copy Markdown
Owner Author

@CodeRabbit review

@coderabbitai

coderabbitai Bot commented Jun 26, 2026 •

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@sonarqubecloud

Copy link
Copy Markdown

@drsnuggles8

Copy link
Copy Markdown
Owner Author

🤖 Self-review (finish-pr) @ 0fd26fe78db0ac1be4d69d5a3a75356a9e141a9d

Reviewed the full PR diff against master at high effort (8 finder angles × verify pass), plus a local build + run of the affected suites.

  • Findings: 0 confirmed. Every candidate the finders raised was refuted against the actual code.
  • Behavior verified: OloEngine-Tests rebuilt clean; ComponentReplicatorTest / ComponentRegistryTest / SnapshotInterpolatorTest / SchedulerConfigTest — 37/37 pass. Matches the 4 green sanitizer CI configs (ASan-MSVC, ASan+LSan, TSan, UBSan) that build and run this suite.
  • Dismissed: "Scale serialization needs a protocol-version guard" (CodeRabbit 🔴) — false positive; the change only reorders the already-existing ar << Scale above the no-op IsLoading() block, wire byte-order unchanged (rebutted + resolved on-thread).
  • Dismissed: "<= 0.0f violates the float-comparison rule" — §2a forbids ==/!=; §2b's own example is value <= 0.0f. Relational compare is endorsed.
  • Dismissed: "SanitizeWireFloat duplicates an existing helper" — no shared fallback-sanitize util exists; per-subsystem std::isfinite guarding is the established convention (SceneSerializer does the same inline).
  • Dismissed: double-guard in SnapshotInterpolator (intentional, documented defense-in-depth), hot-path vec3 temporaries (trivial), pure-parser extraction (deliberate, for unit-testability).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

self-reviewed finish-pr self-review done

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant