Skip to content

GH-1310 Loosen validation for freed objects in dictionary/array#1311

Open
JoltedJon wants to merge 2 commits into
Redot-Engine:masterfrom
JoltedJon:GH-1310
Open

GH-1310 Loosen validation for freed objects in dictionary/array#1311
JoltedJon wants to merge 2 commits into
Redot-Engine:masterfrom
JoltedJon:GH-1310

Conversation

@JoltedJon

@JoltedJon JoltedJon commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Fixes #1310

Dictionary access will completely fail if the key is a node that has been freed has been freed in a typed dictionary. The node is still valid as a key even though it has been freed since the underlying data that is used as the key is still present. This loosens the validation to allow for any thing reads only to use the freed node as a key.

Summary by CodeRabbit

  • Bug Fixes
    • Improved typed key/value validation across dictionary lookups, membership checks, removal, and iteration, using the new lookup-oriented validation behavior.
    • Updated array lookup operations (erase/find/rfind/count/has/bsearch) to use lookup validation consistently, preventing incorrect validation failures in debug scenarios involving previously freed objects.
    • Preserved existing success-path behavior and return values while tightening validation consistency across access patterns.

@JoltedJon
JoltedJon requested review from a team July 16, 2026 01:48
@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

Adds lookup-specific typed container validation that permits previously freed object instances, then applies it across typed array and dictionary lookup, membership, erasure, search, and iteration paths.

Changes

Freed Object Lookup Validation

Layer / File(s) Summary
Lookup validation contract
core/variant/container_type_validate.h
Adds the p_allow_freed validation path and public validate_for_lookup helper, allowing freed object instances during lookup validation.
Array lookup integration
core/variant/array.cpp
Updates typed array erasure, search, counting, membership, and binary search operations to use validate_for_lookup.
Dictionary lookup integration
core/variant/dictionary.cpp
Updates typed dictionary accessors, membership checks, value search, erase, and next to use validate_for_lookup.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: generalprotectionfault

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning Array lookup validation changes are unrelated to the linked dictionary erase issue and appear to be extra scope. Move the array validation changes to a separate PR or link an issue that explicitly covers typed array behavior.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The freed-object lookup change in ContainerTypeValidate and dictionary erase paths addresses the reported typed-dictionary key removal bug.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: loosening freed-object validation in dictionary and array lookup paths.
✨ 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

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@core/variant/dictionary.cpp`:
- Around line 254-256: Update the typed validation used by Array::erase, find,
rfind, count, has, and bsearch, plus Dictionary::find_key, to call the
corresponding lookup validator instead of the regular validate method. Preserve
existing search and return behavior while allowing freed object variants in
these lookup paths.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 59f853dc-4e4d-4f33-b509-595c216bd1f2

📥 Commits

Reviewing files that changed from the base of the PR and between 6bb02aa and 73fabca.

📒 Files selected for processing (2)
  • core/variant/container_type_validate.h
  • core/variant/dictionary.cpp

Comment thread core/variant/dictionary.cpp
@JoltedJon JoltedJon changed the title GH-1310 Loosen validation for freed objects in dictionary GH-1310 Loosen validation for freed objects in dictionary/array Jul 16, 2026
@Shakai-Dev Shakai-Dev added this to the Redot LTS 26.3 milestone Jul 16, 2026

@Shakai-Dev Shakai-Dev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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

Projects

Status: Open

Development

Successfully merging this pull request may close these issues.

Typed Arrays cannot erase pairs with null/invalid keys

2 participants