From ac6a4afb0f6c5a8f443de1c73889ff3a9680b74a Mon Sep 17 00:00:00 2001 From: Scott McKay Date: Tue, 9 Jul 2019 14:59:07 +1000 Subject: [PATCH] Add validation of shape when re-using a buffer in ExecutionFrame (#1356) * Check for empty string as dim_param in allocation planner. * Validate shape is compatible at runtime when re-using Tensor. --- .../core/framework/allocation_planner.cc | 8 +++- onnxruntime/core/framework/execution_frame.cc | 10 +++++ .../test/framework/execution_frame_test.cc | 42 ++++++++++++++++++ .../invalid_dim_param_value_repetition.onnx | Bin 0 -> 2305 bytes 4 files changed, 58 insertions(+), 2 deletions(-) create mode 100644 onnxruntime/test/testdata/invalid_dim_param_value_repetition.onnx diff --git a/onnxruntime/core/framework/allocation_planner.cc b/onnxruntime/core/framework/allocation_planner.cc index 9ffcc9b555..552d702b80 100644 --- a/onnxruntime/core/framework/allocation_planner.cc +++ b/onnxruntime/core/framework/allocation_planner.cc @@ -51,6 +51,7 @@ std::ostream& operator<<(std::ostream& out, std::pair index_to_name; out << "Allocation Plan:\n"; + out << "(ort_value_idx) output_name : \n"; auto plan_size = plan.allocation_plan.size(); for (auto& name_index : session_state.GetOrtValueNameIdxMap()) { @@ -256,8 +257,11 @@ class PlannerImpl { const auto& val2 = shape2.dim(i); if (val1.has_dim_value() && val2.has_dim_value() && (val1.dim_value() == val2.dim_value())) continue; // same known dimension - if (val1.has_dim_param() && val2.has_dim_param() && (val1.dim_param() == val2.dim_param())) - continue; // same unknown dimension + if (val1.has_dim_param() && val2.has_dim_param()) { + const auto& val1_param = val1.dim_param(); + if (val1_param == val2.dim_param() && !val1_param.empty()) + continue; // same unknown dimension + } return false; } return true; diff --git a/onnxruntime/core/framework/execution_frame.cc b/onnxruntime/core/framework/execution_frame.cc index b0831b465e..feae6cddb8 100644 --- a/onnxruntime/core/framework/execution_frame.cc +++ b/onnxruntime/core/framework/execution_frame.cc @@ -316,6 +316,16 @@ Status ExecutionFrame::AllocateMLValueTensorPreAllocateBuffer(OrtValue& ort_valu OrtValue& ort_value_reuse = GetMutableMLValue(ort_value_index_reuse); auto* reuse_tensor = ort_value_reuse.GetMutable(); + + // check number of elements matches. shape may not be an exact match (e.g. Reshape op) + if (reuse_tensor->Shape().Size() != shape.Size()) { + // could be an allocation planner bug (less likely) or the model incorrectly uses something like 'None' + // as a dim_param in multiple places making the planner think those shapes are equal. + return ORT_MAKE_STATUS(ONNXRUNTIME, FAIL, + "Shape mismatch attempting to re-use buffer. ", reuse_tensor->Shape(), " != ", shape, + ". Validate usage of dim_param in shapes in the model."); + } + void* reuse_buffer = reuse_tensor->MutableDataRaw(); // create fence on reused ort_value if needed diff --git a/onnxruntime/test/framework/execution_frame_test.cc b/onnxruntime/test/framework/execution_frame_test.cc index a77ce02805..48e2ce8c3b 100644 --- a/onnxruntime/test/framework/execution_frame_test.cc +++ b/onnxruntime/test/framework/execution_frame_test.cc @@ -6,8 +6,12 @@ #include "core/framework/session_state.h" #include "core/graph/model.h" #include "core/providers/cpu/cpu_execution_provider.h" +#include "core/session/inference_session.h" #include "test_utils.h" +#include "test/test_environment.h" + #include "gtest/gtest.h" +#include "gmock/gmock.h" using namespace ONNX_NAMESPACE; using namespace std; @@ -259,5 +263,43 @@ TEST(ExecutionFrameTest, MemPatternTest) { EXPECT_EQ(p->GetBlock(3)->offset_, 0); EXPECT_EQ(p->GetBlock(4)->offset_, 64); } + +TEST(ExecutionFrameTest, BadModelInvalidDimParamUsage) { + // load model with 2 Scan ops that both incorrectly use shapes of { 'None', 'None' } for their outputs. + // as 'None' is not a special value it's treated as a variable name, leading to a runtime error when we + // attempt to re-use the output from the first Scan node for the second. validate we detect this and error out. + SessionOptions so; + so.session_logid = "BadModelInvalidDimParamUsage"; + + InferenceSession session_object{so, &DefaultLoggingManager()}; + Status st; + ASSERT_TRUE((st = session_object.Load("testdata/invalid_dim_param_value_repetition.onnx")).IsOK()) << st; + ASSERT_TRUE((st = session_object.Initialize()).IsOK()) << st; + + std::vector dims_X = {10, 6}; + std::vector values_X; + values_X.reserve(60); + for (int i = 0; i < 60; ++i) { + values_X.push_back(float(i)); + } + + OrtValue ml_value; + CreateMLValue(TestCPUExecutionProvider()->GetAllocator(0, OrtMemTypeDefault), dims_X, values_X, &ml_value); + NameMLValMap feeds; + feeds.insert(std::make_pair("X", ml_value)); + + // prepare outputs + std::vector output_names; + output_names.push_back("Y"); + std::vector fetches; + + // Now run + RunOptions run_options; + st = session_object.Run(run_options, feeds, output_names, &fetches); + + EXPECT_FALSE(st.IsOK()) << st; + EXPECT_THAT(st.ErrorMessage(), testing::HasSubstr("Shape mismatch attempting to re-use buffer.")); +} + } // namespace test } // namespace onnxruntime diff --git a/onnxruntime/test/testdata/invalid_dim_param_value_repetition.onnx b/onnxruntime/test/testdata/invalid_dim_param_value_repetition.onnx new file mode 100644 index 0000000000000000000000000000000000000000..f7d062463509de04e60c987d98c9b76c7ebb9c40 GIT binary patch literal 2305 zcmeHI%~BIV5Z+A|vJ5CNSQHMRGUY*|#LVm_AyiQU>cIoXG9H9WY9X64C2V4{OMxe! z!YXgPfDhow7r?WxV^2?Z0~@H~;>{d7vpxOI^xxNGk5Rk-=}>lp;Dp(tP%2d6$8%o0 zpj3~H^*^&@ftVj?wpJ{$vHC}^>vR3LZjb^|YPILG=@*@#A9_JpYiX;r0_>kDdso>z zY~r2Yj1K&Ibi{|t{3Nlw6TiP{o|!oS2nBEypv)%T_q?Fr?eu*Dc{^8VnYFCo+t!DF}A1W)svhcx`fPJO{wm@5A;~0w=hAaqf z;I_+VQb7xTMbr`@EW$8`(Jz>FOtFx{8K4HL{X)$%Caxp$6N_G z>RVRjw~B(!^$)zD*pRXc>8yy7TP~HgQe0Lts}7bNW&US|So@vksr->4k4Y}@Pr^n! zpf)Z#QONAqO+N_R;VDm6aPvxHN-Skct!BH`V%d7M&y@g^4T3sq1)bgzn@)-W0ed~4 zrxi)uNkbwKSOd;BlqYe9MpLCBbgBkSirw9tPF$oG$=K%JRP3ty+UF#RkFEW5;`E=2(87Ob5|X108G!-6-rYfF~J_te^;r<`<8 ztzidLMtaJ*NlzKSF7;FaJEVdqmYF^1DREt<$Wwydl4=Y*1i8G%vWmz`Ks56znl;sa z6_$N<(&b8keo^Uha8zifR%8~Kix}i2DUgzpWTF5SllACGd59q8heXOr!E$=2hznDN zqk$kfJywuP60V0&xL!||3$&JT3EWf(B$hyqmcYG93BEobErG-m$YBZOXbA?qUK;RP zmQ~eciCMsDa9=a~dlq9XX_?}7Rk%gBnBulp!ovY{5&y~k56N9fkw<4 zGvO{45nm%`b61c-%4S1E0^1MjfmwL-tw=yV&<=-Bgc zk?#3ba)k*scdwq|m&awE>gVeB`T557;JwbzUC_m3+gns`_H`3;j+nd2sLa>~j9O#c IBF4<*UlDLRSpWb4 literal 0 HcmV?d00001