Repository navigation
Conversation
| 1. Let |descriptor| be a new {{MLOperandDescriptor}}. | ||
| 1. Set |descriptor|.{{MLOperandDescriptor/dataType}} to |dataType|. | ||
| 1. Set |descriptor|.{{MLOperandDescriptor/shape}} to a [=list/clone=] of |shape|. | ||
| 1. If [=MLOperandDescriptor/checking dimensions=] given |descriptor| returns false, then [=exception/throw=] a {{TypeError}}. |
There was a problem hiding this comment.
This is a good fix — it ensures all output operand descriptors are validated, which was missing before.
Since this now applies to every computed output descriptor (not just GRU/LSTM), could you mention that scope in the PR title and commit log? A number of operators — conv2d, gemm, matmul, gather, tile, expand, where, quantize/dequantizeLinear, argMin/argMax, and the broadcasting element-wise ops — gain new TypeError paths here, and reviewers should know that going in.
Two gaps the constructor-side check can't reach:
concat constructs the descriptor from first's shape and then mutates shape[axis] to the accumulated size, so the validated shape isn't the final one. It needs a check after the mutation.
reshape copies input's descriptor and sets shape directly without calling the constructor. The element count is already covered by the existing product check, but the byte length / rank limits aren't.
Could you also audit for any other methods that bypass the constructor or change dimensions afterwards?
There was a problem hiding this comment.
Could you also audit for any other methods that bypass the constructor or change dimensions afterwards?
Done! Fixed the constructor of concat(), pad(), and the pooling ops.
reshape already validates that the output tensor has the same element count as the input tensor, which is ensured to be valid, so it does not need additonal checks.
There was a problem hiding this comment.
Thanks — concat looks right now, and good catch on pad and pool2d.
reshape still copies input.[[descriptor]] and sets shape directly without going through the constructor, so it doesn't get the new validation. As you said, the element count is already covered by the existing product check, but the rank limit isn't — could you convert it to use creating an MLOperandDescriptor for consistency?
|
@shiyi9801 just confirming the shape of this PR is fine and is covered by the existing resolution. After the PR review comments are addressed, this can be merged. Thank you for your contributions that make the API more secure. |
Fix #949
This PR makes
create an MLOperandDescriptorvalidate the resulting shape via checking dimensions, so oversized outputs are rejected during graph building.concat(),pad(), and the pooling ops are restructured to construct their descriptor from the final computed shape instead of mutating one created earlier.This PR also adds unconditional validation for the full-sequence output of
gru()/lstm(), which some platforms produce regardless of thereturnSequenceflag.Preview | Diff