Skip to content

Reject Action Data With Bytes Left After the Arguments - #131

Merged
heifner merged 2 commits into
masterfrom
fix/reject-action-data-trailing-bytes
Nov 10, 2026
Merged

heifner merged 2 commits into
masterfrom
fix/reject-action-data-trailing-bytes

Conversation

@heifner

@heifner heifner commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

The generated dispatcher, SYSIO_DISPATCH and the native-module dispatcher now accept exactly an action's declared arguments, and otherwise fail with action data has bytes left after the arguments. All three call one check in <sysio/dispatcher.hpp>.

  • An action with ignore<T> parameters, taken by value, by const reference or through an alias, reads the rest of its data itself. The dispatcher leaves it to the action, which can finish with sysio::check_action_data_consumed.
  • Notification handlers are not checked. Their data belongs to another contract's action, and a handler may declare only the arguments it uses.

This changes the WASM of every contract built with it, not its ABI. build-pass/dispatch_pinned pins the output.

Pairs with Wire-Network/wire-sysio#755, whose system and test contracts are built with this branch. Merge them together. A wire-sysio tag build uses the latest wire-cdt release, so wire-cdt needs a release before the next wire-sysio tag.

The generated dispatcher, `SYSIO_DISPATCH` and the native-module dispatcher fail an action whose data holds bytes after its declared arguments. An action with `ignore<T>` parameters reads the rest itself and can finish with the new `sysio::check_action_data_consumed`. Notification handlers are not checked, since their data belongs to another contract's action.

Change-Id: Iac34245422cfa991f6f8e953327d81e44c289f5c
@huangminghuang

Copy link
Copy Markdown
Contributor

[P2 / Medium] Preserve the notification exemption in execute_action

At libraries/sysiolib/contracts/sysio/dispatcher.hpp:125–126, execute_action now calls the consumption-checking helper unconditionally. Custom apply() implementations also use execute_action to route notifications, whose handlers may accept only the leading arguments of another contract's action. Those notifications now abort before the handler runs, rolling back the originating transaction, even though generated notification wrappers correctly skip this check.

I reproduced this with a handler void on_notify(name to) invoked through execute_action("receiver"_n, "sender"_n, &notification_contract::on_notify) and a packed (name, name, uint64_t) payload. The same test passes on base 825c71e6 and fails on head 40d209c4 with action data has bytes left after the arguments.

Please make consumption validation conditional on direct action dispatch (self == code) and add a regression test for manual notification routing.

Validation: the new dispatcher unit tests, both new toolchain cases, and focused generated-native-dispatch checks passed. Full chain integration tests were not run.

…ecked

`execute_action` checks for bytes left after the arguments only when the action is the receiver's own (`self == code`). A hand-written `apply()` also routes notifications through it, and a notification handler may declare only the leading arguments of another contract's action. The native-module dispatcher unpacks through the same helper.

Change-Id: I9c06636757f827307d515de849a4eb53145b44ed
@heifner

heifner commented Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

Fixed in 88ea074. The unpack helper behind execute_action and the native-module dispatcher now checks only when self == code, so a notification a hand-written apply() routes through execute_action is not checked. Tests: execute_action_notification_test (unit) and the integration case execute_action_checks_own_actions_only, whose contract routes both a notification and its own action through execute_action. wire-sysio#755 picks up the rebuilt sysmechanics.wasm, the one committed wasm this changes.

@huangminghuang huangminghuang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed the full PR diff at 88ea074, including the follow-up commit. The previous notification finding is fixed: argument-consumption checks run only for the receiver's own actions, and regression coverage includes notifications routed through a hand-written apply(). No remaining actionable findings.

Local validation passed:

  • All six dispatcher unit cases, including execute_action_notification_test.
  • The original notification regression reproducer.
  • Both new toolchain cases: dispatch_ignore_parameters and dispatch_pinned.
  • Regenerated native-dispatch checks for ordinary arguments, ignored arguments, and return values.
  • All four action_data_tests chain integration cases (17 assertions), using contracts freshly compiled against the current PR headers.

Remote CI on this head currently reports failure. The Linux run contains only the failing aggregate gate, and the macOS run contains only the notification job; neither run provides build/test job results.

@heifner
heifner merged commit 4de22af into master Nov 10, 2026
9 of 10 checks passed
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.

2 participants