Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the effects TOML loading logic to better handle “gaps” in configured IDs (eg. missing effectN entries) by preventing get_id() lookups from terminating early when a NamedCommand entry has a NULL .name.
Changes:
- Track the highest loaded ID for effects, effect generators, and effect elements while parsing
effects.toml. - After parsing, attempt to (a) place a NULL sentinel after the highest loaded ID and (b) backfill missing descriptor entries so
get_id()can traverse past gaps.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Set sentinel NULL entry to mark the end of valid entries | ||
| if (max_effect_id >= 0) | ||
| { | ||
| if (max_effect_id + 1 < EFFECTS_TYPES_MAX) | ||
| { | ||
| effect_desc[max_effect_id + 1].name = NULL; | ||
| } |
There was a problem hiding this comment.
The sentinel handling here is unsafe when the highest configured id equals EFFECTS_TYPES_MAX-1: the if (max_effect_id + 1 < EFFECTS_TYPES_MAX) branch won’t run, leaving no guaranteed .name == NULL terminator inside effect_desc[]. Since get_id() iterates until it finds a NULL name, this can lead to out-of-bounds reads. Consider reserving the last slot as the terminator (iterate up to EFFECTS_TYPES_MAX-1 and always set effect_desc[EFFECTS_TYPES_MAX-1].name = NULL), or increasing the array size by 1 to always have room for a sentinel.
| // Set sentinel NULL entry to mark the end of valid entries | ||
| if (max_effectgen_id >= 0) | ||
| { | ||
| if (max_effectgen_id + 1 < EFFECTSGEN_TYPES_MAX) | ||
| { | ||
| effectgen_desc[max_effectgen_id + 1].name = NULL; | ||
| } |
There was a problem hiding this comment.
Same sentinel issue as in load_effects(): if max_effectgen_id reaches EFFECTSGEN_TYPES_MAX-1, no NULL terminator is written within effectgen_desc[], and get_id() can read past the end. Reserve the last element for the sentinel or make the descriptor array one element larger so there’s always space for the terminator.
| // Set sentinel NULL entry to mark the end of valid entries | ||
| if (max_effectelement_id >= 0) | ||
| { | ||
| if (max_effectelement_id + 1 < EFFECTSELLEMENTS_TYPES_MAX) | ||
| { | ||
| effectelem_desc[max_effectelement_id + 1].name = NULL; | ||
| } |
There was a problem hiding this comment.
Same sentinel issue as above: when max_effectelement_id == EFFECTSELLEMENTS_TYPES_MAX-1, the code won’t write any NULL terminator into effectelem_desc[], but get_id() relies on a NULL .name to stop iterating. Please ensure there is always an in-bounds sentinel (reserve the last slot, or allocate the array with an extra element for the terminator).
Re-add sentinel NULL entries and gap placeholder logic for load_effects, load_effectsgenerators, and load_effectelements that was inadvertently removed in the MSVC compatibility commit.
No description provided.