GH-49716: [C++] FixedShapeTensorType::Deserialize should strictly validate serialized metadata by rok · Pull Request #49718 · apache/arrow

rok · 2026-04-13T14:45:47Z

Rationale for this change

FixedShapeTensorType::Deserialize should validate input from unknown sources.

What changes are included in this PR?

Adds stricter deserialization valideation.

Are these changes tested?

Yes. New tests are added.

Are there any user-facing changes?

Stricter validation should not be noticed if metadata is correct as per spec of fixed_shape_tensor.

GitHub Issue: [C++] FixedShapeTensorType::Deserialize should strictly validate serialized metadata #49716

rok · 2026-04-13T14:47:01Z

@pitrou

pitrou · 2026-04-13T15:26:28Z

I suppose VariableShapeTensorType must be similarily improved, right?

pitrou · 2026-04-13T15:27:07Z

@raulcd I think this should be part of 24.0.0.

rok · 2026-04-13T21:27:09Z

I suppose VariableShapeTensorType must be similarily improved, right?

shape is not stored in metadata of VariableShapeTensorType but rather in a field of the struct array (which is storage of variable shape tensor) and could be created invalid. But that wouldn't be checked at deserialization time typically. Is this a check we should do at deserialization time?

We do have uniform_shape which we currently parse and partly validate, but don't really validate. The idea was to potentially use it for optimizations later on/

I added a validation step for permutation.

rok · 2026-04-13T21:29:54Z

@pitrou I pushed requested changes, please do another pass.

kou · 2026-04-14T00:31:43Z

-  for (auto& x : document["shape"].GetArray()) {
+  for (const auto& x : document["shape"].GetArray()) {
+    if (!x.IsInt64()) {
+      return Status::Invalid("shape must contain integers");


Can we show the actual type for easy to debug on failure?

Changed. Now we emit value and type of the value on deserialization failure.

rok · 2026-04-14T09:30:21Z

ping @pitrou

pitrou

Thanks a lot @rok !

raulcd · 2026-04-14T10:31:50Z

I've re-added the R Extra label because it "failed" to run the check-labels workflow. I think there might be a race condition if several labels are applied at the same time and sometimes workflows are cancelled.

rok · 2026-04-14T11:37:26Z

I've restarted a failing R check, others seem to be passing ok.
Edit: Rhub GCC 13 with LTO failed.

raulcd · 2026-04-14T12:11:32Z

That failure is unrelated. Has been failing for some weeks and was tracked in: #49737

rok · 2026-04-14T12:15:12Z

@raulcd shall I rebase and merge if green? Or just merge now?

raulcd

As this has been approved by @pitrou and LGTM, I'll just merge and cherry-pick for 24.0.0. Thanks @rok

…idate serialized metadata (#49718) ### Rationale for this change FixedShapeTensorType::Deserialize should validate input from unknown sources. ### What changes are included in this PR? Adds stricter deserialization valideation. ### Are these changes tested? Yes. New tests are added. ### Are there any user-facing changes? Stricter validation should not be noticed if metadata is correct as per spec of fixed_shape_tensor. * GitHub Issue: #49716 Authored-by: Rok Mihevc <rok@mihevc.org> Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com>

conbench-apache-arrow · 2026-04-14T19:50:21Z

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 41035d4.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 32 possible false positives for unstable benchmarks that are known to sometimes produce them.

conbench-apache-arrow · 2026-04-14T19:51:35Z

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 41035d4.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 32 possible false positives for unstable benchmarks that are known to sometimes produce them.

…ly validate serialized metadata (apache#49718) ### Rationale for this change FixedShapeTensorType::Deserialize should validate input from unknown sources. ### What changes are included in this PR? Adds stricter deserialization valideation. ### Are these changes tested? Yes. New tests are added. ### Are there any user-facing changes? Stricter validation should not be noticed if metadata is correct as per spec of fixed_shape_tensor. * GitHub Issue: apache#49716 Authored-by: Rok Mihevc <rok@mihevc.org> Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com>

initial commit

c4882b1

github-actions Bot added awaiting committer review Awaiting committer review Component: C++ labels Apr 13, 2026

pitrou reviewed Apr 13, 2026

View reviewed changes

Comment thread cpp/src/arrow/extension/fixed_shape_tensor.cc Outdated

pitrou added the Critical Fix Bugfixes for security vulnerabilities, crashes, or invalid data. label Apr 13, 2026

review feedback

afa7022

github-actions Bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Apr 13, 2026

kou reviewed Apr 14, 2026

View reviewed changes

github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Apr 14, 2026

emmit value and type name when failing to deserialize json

b0dcdb7

rok force-pushed the gh-49716-fst-deserialize branch from 3395fec to b0dcdb7 Compare April 14, 2026 09:29

github-actions Bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Apr 14, 2026

pitrou approved these changes Apr 14, 2026

View reviewed changes

pitrou added CI: Extra: C++ Run extra C++ CI CI: Extra: R Run extra R CI labels Apr 14, 2026

pitrou added this to the 24.0.0 milestone Apr 14, 2026

raulcd added CI: Extra: R Run extra R CI and removed CI: Extra: R Run extra R CI labels Apr 14, 2026

raulcd approved these changes Apr 14, 2026

View reviewed changes

raulcd merged commit 41035d4 into apache:main Apr 14, 2026
124 of 156 checks passed

raulcd removed the awaiting changes Awaiting changes label Apr 14, 2026

raulcd mentioned this pull request Apr 14, 2026

[C++] FixedShapeTensorType::Deserialize should strictly validate serialized metadata #49716

Closed

github-actions Bot added the awaiting merge Awaiting merge label Apr 14, 2026

Uh oh!

Conversation

rok commented Apr 13, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

rok commented Apr 13, 2026

Uh oh!

Uh oh!

pitrou commented Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou commented Apr 13, 2026

Uh oh!

rok commented Apr 13, 2026

Uh oh!

rok commented Apr 13, 2026

Uh oh!

kou Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

rok Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rok commented Apr 14, 2026

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

raulcd commented Apr 14, 2026

Uh oh!

rok commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raulcd commented Apr 14, 2026

Uh oh!

rok commented Apr 14, 2026

Uh oh!

raulcd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

conbench-apache-arrow Bot commented Apr 14, 2026

Uh oh!

conbench-apache-arrow Bot commented Apr 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rok commented Apr 13, 2026 •

edited by github-actions Bot

Loading

pitrou commented Apr 13, 2026 •

edited

Loading

rok commented Apr 14, 2026 •

edited

Loading