Skip to content

Fix CPU LSTM input validation and refactor ValidateInputs to frontends - #28653

Closed
Youngsik Yang (vacu9708) wants to merge 2 commits into
microsoft:mainfrom
vacu9708:main
Closed

Youngsik Yang (vacu9708) wants to merge 2 commits into
microsoft:mainfrom
vacu9708:main

Conversation

@vacu9708

@vacu9708 Youngsik Yang (vacu9708) commented May 23, 2026

Copy link
Copy Markdown

Summary

Follow-up to #27737, which fixed out-of-bounds shape reads in the CUDA LSTM path but explicitly deferred the CPU counterpart. This PR addresses CPU LSTM validation gaps.


Problems fixed

1. Structural: validation in the backend instead of the frontend

W's third dimension input_size must equal X's third dimension input_size at runtime,
Validating W therefore depends on X. Beyond this data dependency,
input validation is not the responsibility of the computation. ComputeImpl()
should receive already-validated inputs, not validate them itself.
In addition, ValidateInputs() no longer receives six individual tensors from the caller.

2. Bug: out-of-bounds dimension reads before rank check

In ValidateInputs(), X.Shape()[0] and X.Shape()[1] were read before the NumDimensions() != 3 guard:

// Before — UB on rank < 2
auto& X_shape = X.Shape();
int64_t seq_length = X_shape[0];   // ← read before rank check
int64_t batch_size = X_shape[1];   // ← read before rank check
if (X_shape.NumDimensions() != 3)
  return error;

// After — guard first
if (X_shape.NumDimensions() != 3)
  return error;
int64_t seq_length = X_shape[0];
int64_t batch_size = X_shape[1];

This mirrors the fix applied to the CUDA path in #27737.

3. Missing: CPU LSTM never validated W or R shapes

Neither DeepCpuLstmOp::Compute() nor DynamicQuantizeLSTM::Compute() validated W or R shapes.

DynamicQuantizeLSTM uses transposed W and R layouts.
[num_directions, input_size, 4*hidden_size] and [num_directions, hidden_size, 4*hidden_size] respectively
That's why W/R validation cannot be integrated into ValidateInputs()


Changes

File Change
lstm_base.h ValidateInputs() declaration: (OpKernelContext& context)
lstm_base.cc 1. ValidateInputs() fetches inputs internally.
2. Rank check reordered before dimension reads.
3. call removed from ComputeImpl
deep_cpu_lstm.cc ValidateInputs(*context) + W/R validation called at top of Compute()
dynamic_quantize_lstm.cc Same; W/R validation added for the first time (transposed W layout: [num_directions, input_size, 4*hidden_size])
deep_cpu_lstm_op_test.cc LSTMTest.InvalidInputShapes: wrong-rank W, wrong-dim W, wrong-rank R, wrong-dim R; plus X-rank regression test against ONNX shape inference
quantize_lstm_op_test.cc DynamicQuantLSTMTest.InvalidInputShapes: same four cases for the quantized path

@vacu9708

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

ONNX shape inference (RNNShapeInference) explicitly validates X rank = 3,
but does not validate W or R shapes. The CPU LSTM path had several gaps:

Changes:
- Move ValidateInputs() call from LSTMBase::ComputeImpl() to each frontend
  since W's third dimension must equal X's input_size at runtime.
- Compress ValidateInputs() declaration to eliminate
  redundant input fetching at call sites.
- Reorder X rank check in ValidateInputs() to run before any dimension reads.
- Add W and R shape validation in DeepCpuLstmOp::Compute() and
  DynamicQuantizeLSTM::Compute() before entering the compute pipeline.
    - Note: DynamicQuantizeLSTM uses transposed W layout [num_directions,
    input_size, 4*hidden_size]

Tests:
- Add LSTMTest.InvalidInputShapes covering malformed W and R inputs. The X
  rank sub-case is a regression test for ONNX shape inference behavior; its
  expected error string comes from the ONNX library, not ORT kernel code.
- Add DynamicQuantLSTMTest.InvalidInputShapes covering malformed W and R
  inputs for the quantized LSTM path, including the transposed W layout.

Copilot AI 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.

Pull request overview

This PR closes CPU-side validation gaps for LSTM kernels by moving common input validation to the frontend (Compute()), fixing an out-of-bounds dimension-read hazard in ValidateInputs(), and adding missing runtime shape validation for W and R (including the transposed layout used by DynamicQuantizeLSTM).

Changes:

  • Refactor LSTMBase::ValidateInputs to take OpKernelContext&, fetch inputs internally, and guard X rank before reading dimensions.
  • Move ValidateInputs(*context) calls into CPU LSTM frontends and add explicit W/R shape checks in DeepCpuLstmOp and DynamicQuantizeLSTM.
  • Add negative tests covering invalid W/R ranks and dimensions for both float and dynamic-quantized CPU paths.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
onnxruntime/core/providers/cpu/rnn/lstm_base.h Changes ValidateInputs API to accept OpKernelContext& for frontend-driven validation.
onnxruntime/core/providers/cpu/rnn/lstm_base.cc Implements the refactor; fixes rank-check ordering to avoid OOB reads; removes validation from ComputeImpl.
onnxruntime/core/providers/cpu/rnn/deep_cpu_lstm.cc Calls ValidateInputs up-front and adds missing W/R runtime shape validation for CPU LSTM.
onnxruntime/contrib_ops/cpu/quantization/dynamic_quantize_lstm.cc Calls ValidateInputs up-front and adds W/R runtime shape validation for the transposed quantized layout.
onnxruntime/test/providers/cpu/rnn/deep_cpu_lstm_op_test.cc Adds CPU LSTM invalid-shape tests (including an X-rank regression case).
onnxruntime/test/contrib_ops/quantize_lstm_op_test.cc Adds DynamicQuantizeLSTM invalid-shape tests for W/R.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread onnxruntime/test/providers/cpu/rnn/deep_cpu_lstm_op_test.cc Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI 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.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Comment thread onnxruntime/core/providers/cpu/rnn/lstm_base.cc
Comment thread onnxruntime/test/providers/cpu/rnn/deep_cpu_lstm_op_test.cc
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