Skip to content

Fix Array and CanvasMapper crashes - #65

Open
atkurtul wants to merge 4 commits into
devfrom
array-canvas-mapper-fixes
Open

Fix Array and CanvasMapper crashes#65
atkurtul wants to merge 4 commits into
devfrom
array-canvas-mapper-fixes

Conversation

@atkurtul

@atkurtul atkurtul commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Two crashes reachable by wiring an Array node into a CanvasMapper, plus the faults found while reading around them.

Read canvas layers through the object API

CanvasMapper took the Input array's data view, cast it to a flatbuffers::Vector and called size() on it. Ok() returns null when the pin holds no object, and the view it would otherwise return belongs to a temporary whose guard reference dies with the statement — so the pointer dangled even when it was non-null.

Every layer field now goes through the object API. Also in that loop:

  • the texture of each drawn layer is kept referenced until the pass is submitted, instead of being read from an ObjectRef that died at the end of the iteration;
  • blend mode bits are indexed by the packed draw position rather than the source position, so a skipped layer no longer shifts them onto the wrong layer;
  • the loop stops at the 16 layers the shader declares. It previously ran to the input count and wrote past the end of six stack arrays.

Harden the Array node

Four independent faults, all reachable from the editor:

Fault Effect
SIZE_MAX from a failed name parse used as an index out-of-bounds read on the input vector
GetPin(itemId)->DisplayName unchecked null dereference from the remove menu command
*pin->Object unchecked null dereference during execution
CopyArrayObjectWithEdits(..., &ArrayObject.GetStorage()) one leaked object reference per add and per remove

The leak is the subtle one: writing the new id into the storage of a live ObjectRef overwrites the reference it holds without releasing it. Both edit paths now go through ApplyArrayDelta, which writes to a fresh reference and moves it in, so move-assignment releases the previous one.

Element pins are matched by name rather than by ShowAs. They carry CanShowAs::INPUT_PIN_OR_PROPERTY and are still executed when shown as properties, so filtering on ShowAs would silently drop them.

Notes

  • Both plugins build clean.
  • Not fixed here: GetInputs() stops at the first gap in Input N, so removing a middle element leaves a pin the node cannot see while execution still counts it. It no longer crashes, but the UI count and the array length disagree. Fixing it means choosing between renumbering the survivors and restricting removal to the last element.
  • The related engine-side crash — an empty constructor buffer reaching a resource type's foreign-object constructor — is fixed separately in sys-vulkan.

CanvasMapper took the Input array's data view, cast it to a flatbuffers
vector and called size() on it. Ok() returns null when the pin holds no
object, and the view it would have returned belongs to a temporary whose
guard reference dies with the statement, so feeding the node from an Array
node crashed it.

Read every layer field through the object API instead. The loop also stops
at the 16 layers the shader declares, indexes the blend mode bits by the
packed draw position rather than the source position so a skipped layer no
longer shifts them, and keeps each layer texture referenced until the pass
is submitted.
Four faults, all reachable from the editor. Removing an element whose name
did not parse indexed the input vector with SIZE_MAX. The remove menu
command dereferenced GetPin without checking it found anything. ExecuteNode
dereferenced a pin's object pointer unconditionally. And both edit paths
copied the array into the storage of the ObjectRef that already held the
old array, overwriting the reference without releasing it, so every add and
remove leaked one.

Bounds check the index, check the pin, fail the execution rather than
silently emitting a shorter array, and route both edits through
ApplyArrayDelta, which writes to a fresh reference and moves it in so the
previous one is released. Element pins are matched by name because they can
be shown as properties and are still executed.
The SDK already interprets an object's data view for a trivially copyable type, so the manual buffer handling was a reimplementation.
Drops the local helper now that the SDK reads a trivially copyable field in one call.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant