fix(grass collision): catch trashed actor pointers

[Dawntic]

Fixes a UAF bug when retrieving actor pointers through handles.

[coderabbitai]

📝 Walkthrough

Walkthrough

Replaced uses of actorHandle.get() with actorHandle.getsafe() when collecting actor pointers in grass collision logic; behavior of pushing valid pointers into the actor list is unchanged. No public/exported signatures were modified.

Changes

Cohort / File(s)	Summary
Grass Collision Actor Retrieval src/Features/GrassCollision.cpp	Replaced actorHandle.get() calls with actorHandle.getsafe() for retrieving actor pointers during collision collection; no interface/signature changes and actorList population logic preserved.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Suggested reviewers

• doodlum
• davo0411

Poem

🐇 I sniff the code where grasses sway and peep,
I swap a getter for one that’s safe and neat,
I hop, I nibble bugs away with cheer,
Quiet changes make the meadow clear,
A rabbit smiles — the build stays sweet.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: replacing get() with getsafe() to safely handle trashed actor pointers in grass collision code.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

Using provided base ref: b701bbe
Using base ref: b701bbe
Base commit date: 2026-02-05T18:50:59-08:00 (Thursday, February 05, 2026 06:50 PM)
No actionable suggestions for changed features.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@src/Features/GrassCollision.cpp`:
- Around line 39-49: The per-frame actor loop in GrassCollision.cpp is logging
"Valid actor..." and "End of actor loop..." at info level and spamming logs;
change those info-level success-path logs to a lower-verbosity level (e.g.,
logger::debug or logger::trace) or remove them entirely, while keeping only
error/warning logs for invalid pointer/handle cases (use logger::warn for
invalid actorPtr and actorHandle); locate the logging calls by the symbols
actorPtr.get()->formID, actorPtr.get(), actorHandle.native_handle(), and
actorList.size() and update their logging levels accordingly.

🧹 Nitpick comments (2)

src/Features/GrassCollision.cpp (2)

37-42: Magic pointer-range constant should be documented and named.

The value 0x00007FFFFFFFFFFF is the Windows x64 user-mode address space upper bound. While this heuristic can catch obviously invalid pointers (e.g., uninitialized or corrupted handles), it has limitations:

1. Dangling pointers with valid-looking addresses will still pass this check.
2. The magic number is undocumented and may confuse future maintainers.

Consider extracting this to a named constant with a comment explaining its purpose. Also verify this behaves correctly across SE/AE/VR variants per coding guidelines.

Suggested improvement

+// Windows x64 user-mode address space upper limit; used to detect obviously corrupted pointers
+static constexpr uintptr_t MAX_VALID_USER_ADDR = 0x00007FFFFFFFFFFF;
+
 void GrassCollision::UpdateCollisions(PerFrame& perFrameData)
 {

Then use:

-						if ((uintptr_t)actorPtr.get() < 0x00007FFFFFFFFFFF) {
+						if ((uintptr_t)actorPtr.get() < MAX_VALID_USER_ADDR) {

---

34-46: Defensive checks are well-implemented, but investigate root cause of invalid handles.

The null validation and pointer-range check pattern is sound and properly sourced from po3's PapyrusExtenderSSE (MIT licensed). The 0x00007FFFFFFFFFFF boundary correctly identifies kernel-mode pointers on x64 architecture. The layered checks (handle exists → pointer valid → address in user-mode range) provide solid defense against trashed handles.

However, this is a symptom fix. Consider investigating why highActorHandles contains stale/invalid handles in the first place—this could indicate a deeper issue with actor lifecycle management or handle cache synchronization that may affect other parts of the codebase.

[github-actions]

✅ A pre-release build is available for this PR:
Download

[coderabbitai]

Actionable comments posted: 3

🤖 Fix all issues with AI agents

In `@src/Features/GrassCollision.cpp`:
- Around line 95-99: The code calls CheckForDuplicateActorHandles(processLists)
every frame which walks all actors and allocates containers; change this so
duplicate-handle scanning runs only when diagnostics are enabled or requested
(e.g., guard the call with an IsDiagnosticsEnabled() / diagnostics flag check),
and remove the per-frame allocation by only invoking
CheckForDuplicateActorHandles when that flag is true (update the blocks using
prevAliveActors, prevNumberHighActors, duplicates and any other calls at lines
~95, ~121-123, ~139 to use the same gated check). Ensure you reference
CheckForDuplicateActorHandles and the processLists usage so the scan is skipped
during normal render frames and only executed when diagnostics emission is
active.
- Around line 43-64: The DumpStack function leaks the warning suppression and
calls WinAPI without checking failures; add a matching `#pragma` warning(pop)
after DumpStack and guard GetModuleHandleExA/GetModuleFileNameA calls by
checking their BOOL return values and handling failures (avoid using hModule
when GetModuleHandleExA fails, initialize moduleName or use a fallback string
like "<unknown>" when GetModuleFileNameA fails), and ensure logging uses the
safe filename fallback; reference symbols: DumpStack, CaptureStackBackTrace,
GetModuleHandleExA, GetModuleFileNameA, frames, stack.
- Around line 113-132: The exception thrown after logging an invalid actor
pointer must be removed so the per-frame collision loop in
GrassCollision::Update() (which uses FrameChecker::IsNewFrame()) does not unwind
into the engine; instead, detect the invalid actorPtr/actorHandle, log the
diagnostics (as you already do), and continue by skipping this actor (e.g.,
return/continue out of the inner loop or mark it invalid) rather than throwing;
update the block that references actorPtr, actorHandle, processLists,
DumpStack() to remove the throw std::runtime_error and ensure control flow
safely continues to the next actor.

🧹 Nitpick comments (2)

src/Features/GrassCollision.cpp (2)

23-41: Avoid per-handle vector allocations in duplicate scan.
Only counts are needed; storing index vectors allocates unnecessarily.

♻️ Suggested refactor

-static inline int CheckForDuplicateActorHandles(RE::ProcessLists* processLists)
-{
-	std::unordered_map<RE::Actor*, std::vector<std::uint32_t>> actorToHandles;
-	int totalHandles = 0;
-
-	processLists->ForAllActors([&](RE::Actor* actor) {
-		totalHandles++;
-		actorToHandles[actor].push_back(totalHandles);
-		return RE::BSContainer::ForEachResult::kContinue;
-	});
-
-	int duplicateCount = 0;
-	for (const auto& [actor, handleIndices] : actorToHandles) {
-		if (handleIndices.size() > 1) {
-			duplicateCount++;
-		}
-	}
-	return duplicateCount;
-}
+static inline int CheckForDuplicateActorHandles(RE::ProcessLists* processLists)
+{
+	std::unordered_map<RE::Actor*, std::uint32_t> actorToHandleCounts;
+
+	processLists->ForAllActors([&](RE::Actor* actor) {
+		++actorToHandleCounts[actor];
+		return RE::BSContainer::ForEachResult::kContinue;
+	});
+
+	int duplicateCount = 0;
+	for (const auto& [actor, handleCount] : actorToHandleCounts) {
+		if (handleCount > 1) {
+			++duplicateCount;
+		}
+	}
+	return duplicateCount;
+}

---

72-93: Remove commented-out legacy actor loop (or gate it).
Large commented blocks in hot paths are hard to maintain.

🧹 Suggested cleanup

-	/*
-	if (const auto processLists = RE::ProcessLists::GetSingleton(); processLists) {
-		std::vector<RE::BSTArray<RE::ActorHandle>*> actors;
-		actors.push_back(&processLists->highActorHandles);  // High actors are in combat or doing something interesting
-		for (auto array : actors) {
-			for (auto& actorHandle : *array) {
-				if (actorHandle) {
-					auto actorPtr = actorHandle.get();
-					if (actorPtr && actorPtr.get()) {
-						if ((uintptr_t)actorPtr.get() < 0x00007FFFFFFFFFFF) {
-							actorList.push_back(actorPtr.get());
-						} else {
-							logger::info("[Grass Collision] Invalid actor with raw ptr value: {:X}", (uintptr_t)actorPtr.get());
-						}
-					}
-				} else {
-					logger::info("[Grass Collision] Invalid actor handle with value: {}", actorHandle.native_handle());
-				}
-			}
-		}
-	}
-	*/

[coderabbitai]

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In `@src/Features/GrassCollision.cpp`:
- Around line 114-115: The condition 'if (!actorHandle && actorPtr &&
actorPtr.get())' is contradictory and unreachable; replace the single compound
check with clear separate validations: first check actorHandle and log if it's
invalid (use 'actorHandle' directly), then separately check actorPtr and
actorPtr.get() and log if the pointer is missing/invalid; reference the existing
symbols actorHandle, actorPtr and logger::info (or remove the unreachable
diagnostic altogether if you prefer).
- Line 143: The call to std::min discards its return value so minBodyTimer is
never updated; change the statement to assign the result back to minBodyTimer
(e.g., set minBodyTimer = std::min(minBodyTimer,
actorPtr.get()->GetActorRuntimeData().checkMyDeadBodyTimer)) so minBodyTimer is
properly reduced using the actor's checkMyDeadBodyTimer value.

🧹 Nitpick comments (3)

src/Features/GrassCollision.cpp (3)

23-41: Inefficient duplicate counting implementation.

The function allocates an unordered_map with vector values each call, but only needs to count actors appearing more than once. A simpler approach using unordered_map<RE::Actor*, int> with increment-and-check would avoid per-actor vector allocations.

Additionally, totalHandles is used as an index but the actual value is never used—only handleIndices.size() matters.

♻️ Suggested simplification

 static inline int CheckForDuplicateActorHandles(RE::ProcessLists* processLists)
 {
-	std::unordered_map<RE::Actor*, std::vector<std::uint32_t>> actorToHandles;
-	int totalHandles = 0;
-
-	processLists->ForAllActors([&](RE::Actor* actor) {
-		totalHandles++;
-		actorToHandles[actor].push_back(totalHandles);
-		return RE::BSContainer::ForEachResult::kContinue;
-	});
-
-	int duplicateCount = 0;
-	for (const auto& [actor, handleIndices] : actorToHandles) {
-		if (handleIndices.size() > 1) {
-			duplicateCount++;
-		}
-	}
-	return duplicateCount;
+	std::unordered_map<RE::Actor*, int> actorCounts;
+	int duplicateCount = 0;
+
+	processLists->ForAllActors([&](RE::Actor* actor) {
+		if (++actorCounts[actor] == 2) {
+			duplicateCount++;
+		}
+		return RE::BSContainer::ForEachResult::kContinue;
+	});
+
+	return duplicateCount;
 }

---

72-93: Remove commented-out code before merging.

This block contains the previous implementation that has been replaced by the new validation logic below. Commented-out code should be removed rather than left in the codebase—version control preserves history if needed.

---

111-111: Extract magic number to a named constant.

0x00007FFFFFFFFFFF is the x64 user-mode address space limit but appears as a magic number in two places. Consider defining a named constant for clarity and single-point maintenance.

♻️ Suggested refactor

Add near the top of the file:

static constexpr uintptr_t MAX_VALID_USER_ADDR = 0x00007FFFFFFFFFFF;

Then replace the magic numbers:

-			if ((uintptr_t)actorPtr.get() > 0x00007FFFFFFFFFFF) {
+			if ((uintptr_t)actorPtr.get() > MAX_VALID_USER_ADDR) {

-				if ((uintptr_t)actorPtr.get() < 0x00007FFFFFFFFFFF) {
+				if ((uintptr_t)actorPtr.get() < MAX_VALID_USER_ADDR) {

Also applies to: 137-137

[doodlum]

@Dawntic I think it's probably better to just hook Actor::Update and PlayerCharacter::Update just to cache the collision. (watch out for threading though, maybe). Safe, simple methods are generally the best, so should focus on something where we know for sure is safe. Might not update if game is paused, though. I don't know.

• Hook Actor::Update and PlayerCharacter::Update
• Cache all collisions without using PosAdjust, keep thread safety
• During rendering, lock and read from cache, apply PosAdjust to get the true positions for the GPU shaders
• Clear and unlock cache for next frame

Alternatively could do similar but hook nullsub I think https://github.com/ersh1/TrueDirectionalMovement/blob/85841209d7932051f1f6719160d6ba1af3971c84/src/Hooks.cpp#L1653