Handle RNN activation parameters safely - #31675
Conversation
Co-authored-by: Copilot <223556219@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR tightens attribute validation for the CPU RNN kernel by enforcing expected lengths for activation_alpha and activation_beta, and adds unit tests to ensure invalid lengths fail model loading with clear error messages.
Changes:
- Enforce
activation_alpha/activation_betavector lengths duringRNNkernel construction. - Add a shared test helper plus two new negative tests validating failure behavior for short activation parameter vectors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| onnxruntime/core/providers/cpu/rnn/rnn.h | Adds constructor-time activation_alpha/activation_beta length checks for CPU RNN. |
| onnxruntime/test/providers/cpu/rnn/rnn_op_test.cc | Adds helper + new tests that assert model-load failure and error strings for invalid activation parameter lengths. |
Normalize activation_alpha/activation_beta for the unidirectional default case (2 entries -> 1) to match existing activations handling, and improve length error messages with singular/plural wording. Add regression coverage that unidirectional models accept 2-entry activation_alpha/activation_beta defaults, and update failure-message expectations for the new wording. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Tianlei Wu (tianleiwu)
left a comment
There was a problem hiding this comment.
Thanks for fixing this — the underlying bug is real: Compute() indexes activation_alpha_[direction] / activation_beta_[direction] (rnn.cc:282) for direction < num_directions, so a short attribute list is an out-of-bounds read today.
My main concern is the shape of the fix rather than the intent. An exact-length ORT_ENFORCE is stricter than the ONNX spec and stricter than ORT's own LSTM/GRU kernels, so it can convert models that load today into hard session-creation failures. Details inline.
Summary of findings
- (high) Exact-length validation rejects spec-legal models. ONNX defines
activation_alpha/activation_betaas values "consumed in the order of activation functions" — only activations that actually take an alpha/beta consume one, so the list length need not equalnum_directions. ORT already implements this for LSTM/GRU viarnn::detail::ActivationFuncs/NormalizeActivationArgumentAndGetAlphaBetaCountinrnn_helpers.cc. - (medium) The
size() == 2 -> resize(1)special case silently discards user data and its comment is inaccurate (ONNX defines no default for these attributes, unlikeactivations). - (medium, pre-existing but cemented here)
GetAttrsOrDefault("activation_alpha", std::vector<float>(num_directions, 0.0F))defaults every alpha to0.0. ForLeakyReluthe ONNX default is0.01,Eluis1.0,HardSigmoidis0.2/0.5. Soactivations=["LeakyRelu"]with noactivation_alphasilently computes plainRelu— a bigger correctness problem than the length check, and one that theActivationFuncspath fixes for free. - (minor) Test hygiene — dead DML skip, a test that pins the truncation workaround as a contract, and missing coverage for the empty-list / longer-than-
num_directions/ mixed-activation cases.
Compatibility note: if the strict check is kept as-is, it is a behavioral break for existing models and deserves a release note.
Happy to be pushed back on any of these if I've misread the intended scope.
Reuse the shared activation parameter normalizer so RNN follows ONNX alpha and beta consumption semantics, applies activation-specific defaults, and avoids out-of-bounds reads. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Tianlei Wu (tianleiwu)
left a comment
There was a problem hiding this comment.
Reviewed the final aggregate diff. The previous activation-parameter concerns are addressed: CPU RNN now uses the shared ActivationFuncs normalization path, so alpha/beta values follow ONNX consumption semantics, missing values receive activation-specific defaults, extra entries are safe, and Compute() no longer indexes raw attribute vectors. The focused CPU tests cover mixed activations, empty and omitted parameters, default values, and extra entries. No remaining code-review findings; all required checks pass.
This pull request prevents out-of-bounds reads of RNN activation parameters while preserving ONNX-spec-legal attribute lists.
RNN activation parameter handling:
rnn::detail::ActivationFuncsnormalization already used by ORT's LSTM and GRU kernels.activation_alphaandactivation_betavalues only for activation functions that require them.LeakyRelu,HardSigmoid, andEludefaults.Unit test coverage:
LeakyRelualpha uses the ONNX default of0.01.The tests run explicitly on the CPU execution provider and no longer contain an unrelated DML skip.