fix(PBR): use-after-free CTD

[alandtse]

Summary by CodeRabbit

• Bug Fixes

  • Fixed memory allocation and lifetime management issues in shader materials that could cause crashes.

• Documentation

  • Added clarification on allocation strategies for shader material creation methods.

[coderabbitai]

📝 Walkthrough

Walkthrough

The changes modify allocation strategies for BSLightingShaderMaterialPBR and BSLightingShaderMaterialPBRLandscape factories. Create() methods now allocate via heap new instead of delegating to Make() (scrap-heap), with comprehensive documentation clarifying lifetime/ownership semantics to prevent use-after-free during property linking flows.

Changes

Cohort / File(s)	Summary
Heap Allocation Fixes src/TruePBR/BSLightingShaderMaterialPBR.cpp, src/TruePBR/BSLightingShaderMaterialPBRLandscape.cpp	Create() methods changed from delegating to Make() (scrap-heap) to direct heap allocation via new, altering memory ownership to prevent use-after-free during BSShaderMaterialHashMap::Link and subsequent property freeing.
Allocation Semantics Documentation src/TruePBR/BSLightingShaderMaterialPBR.h, src/TruePBR/BSLightingShaderMaterialPBRLandscape.h	Added detailed comments on Create() and Make() overrides explaining required allocation behaviors: Create() must use heap allocation to avoid freeing canonical copies while property->material still references them; Make() allocates temporary scrap-heap objects for LoadBinary that are replaced during stream linking.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• fix(pbr): use scrap heap allocation #1928: Modifies the same Create()/Make() allocation path for both shader material classes but in the opposite direction, switching between thread scrap-heap and heap allocation strategies.
• fix(truepbr): resolve MATO swap bug #2046: Addresses material lifetime and ownership issues by adding per-material tracking and cloning mechanisms to prevent contamination in pooled materials, complementing the allocation strategy fixes here.

Suggested reviewers

• doodlum

Poem

🐰 Heaps aligned with scrap now clear,
No more dangling pointers here!
Materials bound in proper place,
Free-safe allocation—balanced grace. ✨

🚥 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
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly and specifically addresses the main change: fixing a use-after-free crash (CTD) in the PBR material allocation logic by changing allocation from scrap-heap to regular heap.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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]

No actionable suggestions for changed features.

[alandtse]

@coderabbitai critically review this PR to confirm whether it should fix the purported ctd. Also check that this doesn't cause a regression based on the history.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Read the modified .cpp files
echo "=== BSLightingShaderMaterialPBR.cpp ==="
fd -e cpp BSLightingShaderMaterialPBR --exec cat {}

echo ""
echo "=== BSLightingShaderMaterialPBRLandscape.cpp ==="
fd -e cpp BSLightingShaderMaterialPBRLandscape --exec cat {}

echo ""
echo "=== BSLightingShaderMaterialPBR.h ==="
fd -e h BSLightingShaderMaterialPBR.h --exec cat {}

echo ""
echo "=== BSLightingShaderMaterialPBRLandscape.h ==="
fd -e h BSLightingShaderMaterialPBRLandscape.h --exec cat {}

Length of output: 176

---

🏁 Script executed:

#!/bin/bash
# Try finding the files with find
find . -name "BSLightingShaderMaterial*" -type f 2>/dev/null

echo ""
echo "=== Try tree for TruePBR directory ==="
find . -path "*/TruePBR*" -type f 2>/dev/null | head -50

echo ""
echo "=== Git diff for this PR ==="
git diff HEAD~1 HEAD -- "*.cpp" "*.h" | head -300

Length of output: 5567

---

🏁 Script executed:

#!/bin/bash
# Get full content of the cpp files from git (HEAD = PR branch)
echo "=== BSLightingShaderMaterialPBR.cpp (full, HEAD) ==="
git show HEAD:src/TruePBR/BSLightingShaderMaterialPBR.cpp

echo ""
echo "=== BSLightingShaderMaterialPBRLandscape.cpp (full, HEAD) ==="
git show HEAD:src/TruePBR/BSLightingShaderMaterialPBRLandscape.cpp

echo ""
echo "=== BSLightingShaderMaterialPBR.h (full, HEAD) ==="
git show HEAD:src/TruePBR/BSLightingShaderMaterialPBR.h

echo ""
echo "=== BSLightingShaderMaterialPBRLandscape.h (full, HEAD) ==="
git show HEAD:src/TruePBR/BSLightingShaderMaterialPBRLandscape.h

Length of output: 29277

---

@alandtse — here is my critical analysis:

---

Verdict: ✅ Fix is correct and should resolve the CTD

Root Cause Confirmation

The UAF chain is well-understood and the fix directly targets it:

LoadBinary
  └─ Make()  → scrap-heap temp → property->material = temp

LinkObject (NiStream link phase)
  ├─ BSShaderMaterialHashMap::Link
  │     └─ Create() [OLD: return Make()] → canonical also on scrap heap ← BUG
  │           CopyMembers(temp → canonical)
  └─ ScrapHeap::Free()  → pops scrap heap stack
        ├─ frees temp  ✓
        └─ [OLD] also frees canonical ✗ → property->material now dangling → CTD

After the fix, Create() returns new BSLightingShaderMaterialPBR/PBRLandscape(), so the canonical lives on the regular heap. ScrapHeap::Free() only pops the temp — the canonical survives, and property->material remains valid.

---

Regression Analysis

Risk	Assessment
Double-free	None. Temp is freed by ScrapHeap::Free(); canonical by engine ref-count via DeleteThis(). Two distinct allocations, two distinct frees.
Constructor initialization	new BSLightingShaderMaterialPBR() vs old memset + construct_at. All fields have in-class default initializers (coatRoughness = 1.f, NiPointer → null, etc.), so new produces a correctly zero/default-initialized object. No behavioral difference.
BSLightingShaderMaterialPBRLandscape() ctor	Explicit constructor (initializes isPbr, roughnessScales, displacementScales, specularLevels) is properly invoked by new. ✓
All map management	CopyMembers does All[canonical] = All[temp], so the canonical gets correct extension data. Any stale All[temp] entry after scrap-heap pop is a pre-existing issue not introduced here.
Symmetry	Both BSLightingShaderMaterialPBR and BSLightingShaderMaterialPBRLandscape are fixed identically. No half-fixed state.
DeleteThis() for heap objects	Neither class overrides DeleteThis(), so they fall back to RE::BSLightingShaderMaterialBase. Since Create() returning a new-allocated object is the intended engine contract for the virtual factory, the base DeleteThis() is expected to handle it correctly with delete this.

---

One Minor Observation (non-blocking)

There is no DeleteThis() override in either PBR material class. This is fine as long as the base RE::BSLightingShaderMaterialBase::DeleteThis() dispatches to delete this (the standard CommonLibSSE pattern for Create()-produced canonicals). If you ever see a secondary memory issue, that would be the first place to check — but there's no evidence it's a problem here.

---

Summary: The PR correctly identifies and fixes the root cause with minimal, symmetric changes. The Create() / Make() separation is now architecturally sound and well-documented. I'd recommend merging.

---

🧠 Learnings used

Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 2000
File: src/WeatherEditor/EditorWindow.cpp:1271-1274
Timestamp: 2026-03-22T18:40:49.633Z
Learning: Repo: doodlum/skyrim-community-shaders — Maintainer preference (PR `#2000` on 2026-03-22): In src/WeatherEditor/EditorWindow.cpp within EditorWindow::RenderUI, keep `resetLayout = false;` at the end of the frame (after PaletteWindow::Draw). The reset is intentionally one-shot for windows rendered that frame; closed windows are allowed to reopen with their previous geometry. Do not move the clear later or broadcast resets to closed windows.

---

Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.

---

Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 1950
File: package/Shaders/Lighting.hlsl:2008-2012
Timestamp: 2026-03-07T00:53:21.267Z
Learning: In doodlum/skyrim-community-shaders PR `#1950`, the maintainer (Dlizzio) confirmed that per-axis normal reorientation for triplanar-projected normals/detail in package/Shaders/Lighting.hlsl is not required for their use case; the current approach intentionally mixes samples without axis-specific swizzle/sign correction.

---

Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 1947
File: package/Shaders/DeferredCompositeCS.hlsl:234-243
Timestamp: 2026-03-14T08:35:42.651Z
Learning: In `package/Shaders/DeferredCompositeCS.hlsl` (doodlum/skyrim-community-shaders), the fallback non-IBL SKYLIGHTING specular path intentionally double-weights with `skylightingSpecular`: `dalcScaled` is computed as `IrradianceToGamma(IrradianceToLinear(directionalAmbientColorSpecular) * skylightingSpecular)`, and the final `finalIrradiance` is then `lerp(specularIrradiance, specularIrradianceReflections, skylightingSpecular)`. This is original engine logic preserved from before PR `#1947` and should not be flagged as a double-attenuation bug.

---

Learnt from: SkrubbySkrubInAShrub
Repo: doodlum/skyrim-community-shaders PR: 2143
File: src/WeatherEditor/WeatherUtils.cpp:35-41
Timestamp: 2026-04-18T12:11:13.746Z
Learning: In doodlum/skyrim-community-shaders (PR `#2143`), `WeatherUtils::TexturePath::BuildResourcePath` in `src/WeatherEditor/WeatherUtils.cpp` is intentionally designed for relative texture paths only (no leading `textures\\` prefix). Callers always supply prefix-free paths, so the `"Textures\\"` prepend never produces duplicate segments.

---

Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:277-293
Timestamp: 2025-08-05T18:22:40.578Z
Learning: In the skyrim-community-shaders SnowCover feature, the wstrtostr and strtowstr utility functions defined in src/Features/SnowCover.cpp are unused and should be removed rather than fixed, as confirmed by the original author ThePagi.

[github-actions]

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