fix: heap corruption by SimplisticMind · Pull Request #1895 · community-shaders/skyrim-community-shaders

SimplisticMind · 2026-02-17T08:52:43Z

Partially fixes heap corruption; I don't have ability to test PBR materials. Tested by using AppVerifier and going ingame.
Fixes #1654

Summary by CodeRabbit

Refactor
- Changed how materials are allocated for non-menu screens by switching to a different allocation mechanism; allocation for menu screens remains unchanged. Overall material linking and runtime behavior are preserved.

coderabbitai · 2026-02-17T08:53:06Z

No actionable comments were generated in the recent review. 🎉

📝 Walkthrough

Walkthrough

Replaced direct material allocation in the non-menu branch of BSLightingShaderProperty_LoadBinary with a relocation-resolved CreateOnScrapHeap call that allocates the material on Skyrim's Thread ScrapHeap (8-byte alignment); menu-screen path and subsequent linking/state updates remain unchanged.

Changes

Cohort / File(s)	Summary
Material allocation change `src/TruePBR.cpp`	Replaced `RE::BSLightingShaderMaterialBase::CreateMaterial(feature)` with a relocation-resolved `CreateOnScrapHeap` invocation for non-menu-screen material allocation (uses Thread ScrapHeap, align=8). Control flow, linking, and state updates unchanged; menu branch unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Suggested reviewers

alandtse
doodlum

Poem

🐇 I dug a tiny heap below,
Where eight-byte dreams can safely grow.
Materials snug in scrapbed light,
I hopped away — the pointers right.
A happy hop, the code says so.

🚥 Pre-merge checks | ✅ 4 | ❌ 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 (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix: heap corruption' directly matches the primary objective in the linked issue `#1654`, clearly summarizing the main change in addressing heap corruption.
Linked Issues check	✅ Passed	The code change replaces direct material allocation with a relocation-based CreateOnScrapHeap function pointer call, which addresses the heap corruption issue identified in `#1654` by changing how memory is allocated for shader materials.
Out of Scope Changes check	✅ Passed	The changes are focused solely on modifying the allocation mechanism in BSLightingShaderProperty_LoadBinary, which is directly scoped to fixing the heap corruption issue identified in `#1654`.

_{✏️ 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

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

github-actions · 2026-02-17T08:53:45Z

Using provided base ref: a056641
Using base ref: a056641
Base commit date: 2026-02-19T14:36:45Z (Thursday, February 19, 2026 02:36 PM)
No actionable suggestions for changed features.

github-actions · 2026-02-17T09:12:18Z

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

alandtse · 2026-02-17T17:49:40Z

Thanks. Perhaps this needs to be fixed on the comonlib side actually. I'll investigate since it is using some of our internal RE (that should be upstream instead).

alandtse · 2026-02-17T17:50:39Z

@doodlum seems plausible, but thoughts?

doodlum · 2026-02-17T18:04:43Z

i thought clib fixed this. this method looks fine, just the relid should be in globals

alandtse · 2026-02-17T18:21:21Z

i thought clib fixed this. this method looks fine, just the relid should be in globals

@doodlum Was this your PR? We should be on it now.

doodlum · 2026-02-17T18:29:46Z

yeah

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/TruePBR.cpp (1)
569-572: Commit title: add a scope for clarity

The current title fix: heap corruption follows conventional commits but the scope is absent. A scoped title more precisely conveys the change:
fix(pbr): use scrapheap for material allocation in LoadBinary
As per coding guidelines on Conventional Commit Titles: type(scope): description, max 50 characters.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/TruePBR.cpp` around lines 569 - 572, Update the commit title to include a
scope and concise description following Conventional Commits (type(scope):
description) so it reads like "fix(pbr): use scrapheap for material allocation
in LoadBinary" (keep under ~50 chars); this change is purely the commit message
for the change around
CreateOnScrapHeap/RE::BSLightingShaderMaterialBase::CreateMaterial allocation in
LoadBinary where material is assigned, so amend the existing commit message (git
commit --amend or interactive rebase) to the scoped title and push/update the
PR.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/TruePBR.cpp`:
- Around line 562-567: The PBR branch is allocating BSLightingShaderMaterialPBR
via BSLightingShaderMaterialPBR::Make() which currently uses RE::malloc with
align=0 instead of allocating from the Thread ScrapHeap with align=8, leaving
LinkObject/Deallocate vulnerable to crashes; update the allocation path for
BSLightingShaderMaterialPBR (the Make/constructor path used in the
kMenuScreen/PBR branch) to allocate from the Thread ScrapHeap with align=8
(matching the fix applied to the other material branch), ensure any
corresponding deallocation uses the ScrapHeap-aware Deallocate/LinkObject logic,
and add a short comment referencing Thread ScrapHeap align=8 for future
reviewers.

---

Nitpick comments:
In `@src/TruePBR.cpp`:
- Around line 569-572: Update the commit title to include a scope and concise
description following Conventional Commits (type(scope): description) so it
reads like "fix(pbr): use scrapheap for material allocation in LoadBinary" (keep
under ~50 chars); this change is purely the commit message for the change around
CreateOnScrapHeap/RE::BSLightingShaderMaterialBase::CreateMaterial allocation in
LoadBinary where material is assigned, so amend the existing commit message (git
commit --amend or interactive rebase) to the scoped title and push/update the
PR.

alandtse · 2026-02-22T01:33:31Z

 		{
 			RE::BSLightingShaderMaterialBase* material = nullptr;
 			if (property->flags.any(kMenuScreen)) {
+				// Note: Same bug as below, should be using Thread ScrapHeap with align=8 instead of RE::malloc with align=0


@claude can you please add the implementation as needed? Please also determine if this fix should actually be PR upstream to commonlib which we will sycn with via our commonlib. https://github.com/powerof3/CommonLibSSE

alandtse · 2026-02-22T05:49:54Z

Ok, confirmed this is fixed from commonlib already. Will do another PR to handle the PBR path.

@SimplisticMind thanks for filing the issue and the pr.

PBR material Make() was using operator new (via TES_HEAP_REDEFINE_NEW), which allocates from the global heap with RE::malloc. The game expects materials to be allocated from the Thread ScrapHeap with align=8. This mismatch causes heap corruption when materials are later freed via scrap heap deallocation paths. Switch to scrapHeap->Allocate + std::construct_at, matching the pattern used by CommonLib's BSLightingShaderMaterialBase::CreateMaterial Ref: community-shaders#1895, community-shaders#1654

doodlum approved these changes Feb 18, 2026

View reviewed changes

coderabbitai Bot reviewed Feb 18, 2026

View reviewed changes

Comment thread src/TruePBR.cpp

fix: heap corruption

b5e46cf

SimplisticMind force-pushed the dev branch from dce94be to b5e46cf Compare February 18, 2026 22:37

coderabbitai Bot mentioned this pull request Feb 18, 2026

fix: heap corruption in BSLightingShaderMaterialPBR allocation (kMenuScreen/PBR path) #1910

Closed

doodlum added 3 commits February 19, 2026 12:36

Merge branch 'dev' into dev

b4e6c0d

Merge branch 'dev' into dev

3283143

Merge branch 'dev' into dev

75a12ed

alandtse reviewed Feb 22, 2026

View reviewed changes

alandtse closed this Feb 22, 2026

alandtse mentioned this pull request Feb 22, 2026

fix(pbr): use scrap heap allocation #1928

Merged

Conversation

SimplisticMind commented Feb 17, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Suggested reviewers

Poem

❌ Failed checks (1 warning)

Uh oh!

github-actions Bot commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alandtse commented Feb 17, 2026

Uh oh!

alandtse commented Feb 17, 2026

Uh oh!

doodlum commented Feb 17, 2026

Uh oh!

alandtse commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

doodlum commented Feb 17, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alandtse Feb 22, 2026

Choose a reason for hiding this comment

Uh oh!

alandtse commented Feb 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SimplisticMind commented Feb 17, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Feb 17, 2026 •

edited

Loading

github-actions Bot commented Feb 17, 2026 •

edited

Loading

github-actions Bot commented Feb 17, 2026 •

edited

Loading

alandtse commented Feb 17, 2026 •

edited

Loading