fix(baremetal): bound every I/O slot write, and stop truncating Modbus sizes - #1079
Merged
marconetsf merged 1 commit intoSep 18, 2026
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
9 tasks
…s sizes Three defects in the bare-metal firmware's I/O path, all reachable today and all independent of how the process image is sized. **Out-of-bounds writes when binding located variables.** runtime_bind_located_vars() wrote bool_input / bool_output / int_input / int_output / *_memory at whatever byte_index the descriptor carried. Only the DWord cases were bounded. `%QX7.0` against the 56-output image indexes bool_output[7][8] -- one past the end -- and corrupts whatever follows it. The editor allocates and compiles that address without complaint, so nothing upstream stops it either. Every slot write is now range-checked; an out-of-range descriptor is skipped, leaving the slot NULL, which every HAL and the Modbus glue already treat as "not wired". **Modbus bank sizes truncated to 8 bits.** init_mbregs() took its six sizes as uint8_t and MBinfo stored them the same way, so any bank above 255 wrapped silently -- 256 coils became 0, and the register map came up wrong with no diagnostic. Widened to uint16_t, along with the byte_addr in get_discrete/write_discrete (addr/8 overflows a uint8_t past 2040 coils) and the pos index into dint_memory/lint_memory. **readCoils aliased every coil above 255.** It called get_discrete((uint8_t)startreg) on a 16-bit coil address, so FC 0x01 answered with the wrong bit for anything past 255 -- silently, no exception. readInputStatus never had the cast. Harmless while no board had more than 56 coils; a bug the moment one does. Also replaces mapEmptyBuffers()'s malloc(1)-per-unbound-discrete-point with a single static block. PLC firmware avoids the heap on principle -- allocation there fragments and never recovers -- and one array costs exactly MAX_DIGITAL_INPUT + MAX_DIGITAL_OUTPUT bytes with no allocator involved. Refs openplc-editor#296, which is what surfaced these. The process-image sizing itself is a separate piece of work and is not in this PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
JulioSergioFS
force-pushed
the
bugfix/gh-296-baremetal-io-bounds
branch
from
September 2, 2026 23:46
7b2ab67 to
f050b30
Compare
marconetsf
changed the base branch from
development
to
task/DOPE-615-size-the-io-image-from-the-project
September 18, 2026 09:43
marconetsf
merged commit Sep 18, 2026
6fb887e
into
task/DOPE-615-size-the-io-image-from-the-project
11 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull request info
References
Refs #296 — these are the defects that issue surfaced. The process-image sizing itself is not here (see Scope below).
Paired mirror PR: Autonomy-Logic/openplc-web#729 — merge together,
ci-syncneeds both.Supersedes the firmware half of #1069, which is being closed.
Scope
#296 asked why the P1AM-200 has the same I/O ceiling as the P1AM-100, given it has 3x the memory.
The first attempt (now closed: #1069, openplc-packages#46) let each device declare its own image size in its VPP manifest. Review set that aside, and the reason is worth recording: the board's memory is for everything — the user's program, its variables, every buffer — not just I/O. Reserving 3x more image on the -200 eats memory the program needs, and most projects never use that much I/O. It also means guessing a number per board, forever, for boards nobody can measure.
The decision is that the image is sized by the user's program instead: if the program declares 200 I/Os, 200 are allocated. That is not
malloc— the compiler knows the size at build time, so on bare metal it becomes a generated#define. The same fixed-image problem exists in Runtime v4 and is being addressed there too. That work belongs to the Modbus Server unification (DOPE-370 / DOPE-371) and is not in this PR.This PR carries only the part that is a bug regardless of how the image is sized, and that is worth fixing on its own.
Description of the changes proposed
Out-of-bounds writes when binding located variables.
runtime_bind_located_vars()wrotebool_input/bool_output/int_*/*_memoryat whateverbyte_indexthe descriptor carried. Only theDWordcases were bounded.%QX7.0against the 56-output image indexesbool_output[7][8]— one past the end — and corrupts whatever follows it. The editor allocates and compiles that address without complaint, so nothing upstream stops it either.Every slot write is now range-checked. An out-of-range descriptor is skipped, leaving the slot
NULL, which every HAL and the Modbus glue already treat as "not wired".Modbus bank sizes truncated to 8 bits.
init_mbregs()took its six sizes asuint8_tandMBinfostored them the same way, so any bank above 255 wrapped silently — 256 coils became 0, and the register map came up wrong with no diagnostic. Widened touint16_t, along withbyte_addringet_discrete/write_discrete(addr/8overflows auint8_tpast 2040 coils) and theposindex intodint_memory/lint_memory.readCoilsaliased every coil above 255. It calledget_discrete((uint8_t)startreg)on a 16-bit coil address, so FC 0x01 answered with the wrong bit for anything past 255 — silently, no exception.readInputStatusnever had the cast. Harmless while no board had more than 56 coils; a bug the moment one does.mapEmptyBuffers()no longer allocates per point. It calledmalloc(1)once per unbound discrete point. PLC firmware avoids the heap on principle — allocation there fragments and never recovers — and one static array costs exactlyMAX_DIGITAL_INPUT + MAX_DIGITAL_OUTPUTbytes with no allocator involved.Note for whoever picks up the sizing work
The
openplc.hchange that makesMAX_*overridable (#ifndefguards +#include "defines.h") is deliberately not here — it is the enabling mechanism for the sizer, which is someone else's task now. It exists ready to reuse on the branchfeature/gh-296-gh-565-process-image-and-located-arrays.DOD checklist
Verification
tsc --noEmit— 0 errors;jest src/backend/shared/compilegreen (nothing in TypeScript changed, run as a regression guard).compare-surfaces.pyagainst web#729 —match: True, 0 diffs.Not verified — needs hardware
No board was flashed. Two things want a real check before this is trusted in the field:
🤖 Generated with Claude Code