From f4cb9a89b2bbc7c546c198b8c5b905f06aba3aff Mon Sep 17 00:00:00 2001 From: "Ahmad Hassan (B-Ted)" Date: Mon, 9 Nov 2026 23:33:28 +0500 Subject: [PATCH 1/2] fix(runtime): guard null operator overload and named data key dereferences Guard against null pointer dereferences when logging KernelCall errors in Method::execute_instruction and when resolving keys in FlatTensorDataMap. - runtime/executor/method.cpp: Check op and op->overload() before invoking c_str() in the KernelCall failure log. Operators with no overload string now format cleanly as op_name without segfaulting. - extension/flat_tensor/flat_tensor_data_map.cpp: In get_named_data, verify entry and entry->key() are non-null before comparison. In get_key, validate index bounds and check entry and entry->key() are non-null before returning, returning InvalidExternalData on malformed entries instead of faulting. - extension/flat_tensor/test/flat_tensor_data_map_test.cpp: Add test coverage for out-of-bounds key access. Fixes #23955. --- .../flat_tensor/flat_tensor_data_map.cpp | 34 +++++++++++++++---- .../test/flat_tensor_data_map_test.cpp | 3 ++ runtime/executor/method.cpp | 13 +++++-- 3 files changed, 41 insertions(+), 9 deletions(-) diff --git a/extension/flat_tensor/flat_tensor_data_map.cpp b/extension/flat_tensor/flat_tensor_data_map.cpp index 6cdabc6f401..7d6267968a4 100644 --- a/extension/flat_tensor/flat_tensor_data_map.cpp +++ b/extension/flat_tensor/flat_tensor_data_map.cpp @@ -63,12 +63,16 @@ Result get_named_data( return Error::InvalidExternalData; } for (flatbuffers::uoffset_t i = 0; i < named_data->size(); ++i) { - if (key.size() == named_data->Get(i)->key()->size() && + const auto* entry = named_data->Get(i); + if (entry == nullptr || entry->key() == nullptr) { + continue; + } + if (key.size() == entry->key()->size() && std::strncmp( - named_data->Get(i)->key()->c_str(), + entry->key()->c_str(), key.data(), - named_data->Get(i)->key()->size()) == 0) { - const auto* found = named_data->Get(i); + entry->key()->size()) == 0) { + const auto* found = entry; // Validate the named_data. const flatbuffers::uoffset_t segment_index = found->segment_index(); ET_CHECK_OR_RETURN_ERROR( @@ -249,19 +253,37 @@ ET_NODISCARD Error FlatTensorDataMap::load_data_into( } ET_NODISCARD Result FlatTensorDataMap::get_num_keys() const { + if (flat_tensor_ == nullptr || flat_tensor_->named_data() == nullptr) { + return 0; + } return flat_tensor_->named_data()->size(); } ET_NODISCARD Result FlatTensorDataMap::get_key( uint32_t index) const { - uint32_t num_keys = get_num_keys().get(); + Result num_keys_res = get_num_keys(); + if (!num_keys_res.ok()) { + return num_keys_res.error(); + } + uint32_t num_keys = num_keys_res.get(); ET_CHECK_OR_RETURN_ERROR( index < num_keys, InvalidArgument, "Index %" PRIu32 " out of range of size %" PRIu32, index, num_keys); - return flat_tensor_->named_data()->Get(index)->key()->c_str(); + const auto* named_data = flat_tensor_->named_data(); + ET_CHECK_OR_RETURN_ERROR( + named_data != nullptr, + InvalidExternalData, + "Named data is null"); + const auto* entry = named_data->Get(index); + ET_CHECK_OR_RETURN_ERROR( + entry != nullptr && entry->key() != nullptr, + InvalidExternalData, + "Named data entry or key at index %u is null", + index); + return entry->key()->c_str(); } /* static */ Result FlatTensorDataMap::load( diff --git a/extension/flat_tensor/test/flat_tensor_data_map_test.cpp b/extension/flat_tensor/test/flat_tensor_data_map_test.cpp index d628594b8eb..1ba292014a4 100644 --- a/extension/flat_tensor/test/flat_tensor_data_map_test.cpp +++ b/extension/flat_tensor/test/flat_tensor_data_map_test.cpp @@ -134,6 +134,9 @@ TEST_F(FlatTensorDataMapTest, GetKeys) { // Check get_key fails when out of bounds. Result key2_res = data_map->get_key(2); EXPECT_EQ(key2_res.error(), Error::InvalidArgument); + EXPECT_EQ( + data_map->get_key(std::numeric_limits::max()).error(), + Error::InvalidArgument); } TEST_F(FlatTensorDataMapTest, LoadInto) { diff --git a/runtime/executor/method.cpp b/runtime/executor/method.cpp index e512081d6ff..65d4688b5de 100644 --- a/runtime/executor/method.cpp +++ b/runtime/executor/method.cpp @@ -1732,14 +1732,21 @@ Error Method::execute_instruction() { // checked at init time. auto op_index = instruction->instr_args_as_KernelCall()->op_index(); ET_UNUSED auto op = serialization_plan_->operators()->Get(op_index); + const char* const op_name = + (op != nullptr && op->name() != nullptr) ? op->name()->c_str() : "unknown"; + const bool has_overload = + op != nullptr && op->overload() != nullptr && op->overload()->size() > 0; + const char* const op_overload = + has_overload ? op->overload()->c_str() : ""; ET_LOG( Error, "KernelCall failed at instruction %" ET_PRIsize_t ":%" ET_PRIsize_t - " in operator %s.%s: 0x%x", + " in operator %s%s%s: 0x%x", step_state_.chain_idx, step_state_.instr_idx, - op->name()->c_str(), - op->overload()->c_str(), + op_name, + has_overload ? "." : "", + op_overload, (unsigned int)err); for (size_t i = 0; i < args.size(); ++i) { ET_LOG( From cea98b169ddcda8d12091310888bd180d8940a6e Mon Sep 17 00:00:00 2001 From: "Ahmad Hassan (B-Ted)" Date: Wed, 11 Nov 2026 09:19:40 +0500 Subject: [PATCH 2/2] fix(runtime): address review feedback on null overload logging and named data errors - Inline operator overload null check in ET_LOG to prevent unused variable warnings when logging is compiled out. - Return InvalidExternalData for keyless or null named data entries in get_named_data and get_key. - Simplify get_num_keys and remove unreachable null check branch. - Add test coverage for keyless named data entries in flat_tensor_data_map_test. - Add test coverage for kernel failure with null operator overload in program_validation_test. --- .../flat_tensor/flat_tensor_data_map.cpp | 41 +++++------- .../test/flat_tensor_data_map_test.cpp | 60 +++++++++++++---- runtime/executor/method.cpp | 15 ++--- runtime/executor/test/CMakeLists.txt | 13 ++++ .../executor/test/program_validation_test.cpp | 65 ++++++++++++++++++- 5 files changed, 146 insertions(+), 48 deletions(-) diff --git a/extension/flat_tensor/flat_tensor_data_map.cpp b/extension/flat_tensor/flat_tensor_data_map.cpp index 7d6267968a4..3d8dac64232 100644 --- a/extension/flat_tensor/flat_tensor_data_map.cpp +++ b/extension/flat_tensor/flat_tensor_data_map.cpp @@ -64,17 +64,17 @@ Result get_named_data( } for (flatbuffers::uoffset_t i = 0; i < named_data->size(); ++i) { const auto* entry = named_data->Get(i); - if (entry == nullptr || entry->key() == nullptr) { - continue; - } + ET_CHECK_OR_RETURN_ERROR( + entry != nullptr && entry->key() != nullptr, + InvalidExternalData, + "Named data entry or key at index %" PRIu32 + " is null. Malformed PTD file.", + i); if (key.size() == entry->key()->size() && - std::strncmp( - entry->key()->c_str(), - key.data(), - entry->key()->size()) == 0) { - const auto* found = entry; + std::strncmp(entry->key()->c_str(), key.data(), entry->key()->size()) == + 0) { // Validate the named_data. - const flatbuffers::uoffset_t segment_index = found->segment_index(); + const flatbuffers::uoffset_t segment_index = entry->segment_index(); ET_CHECK_OR_RETURN_ERROR( segment_index < segments->size(), InvalidExternalData, @@ -98,7 +98,7 @@ Result get_named_data( segments->Get(segment_index)->offset(), segments->Get(segment_index)->size(), segment_data_size); - return found; + return entry; } } return Error::NotFound; @@ -253,35 +253,24 @@ ET_NODISCARD Error FlatTensorDataMap::load_data_into( } ET_NODISCARD Result FlatTensorDataMap::get_num_keys() const { - if (flat_tensor_ == nullptr || flat_tensor_->named_data() == nullptr) { - return 0; - } return flat_tensor_->named_data()->size(); } ET_NODISCARD Result FlatTensorDataMap::get_key( uint32_t index) const { - Result num_keys_res = get_num_keys(); - if (!num_keys_res.ok()) { - return num_keys_res.error(); - } - uint32_t num_keys = num_keys_res.get(); + const auto* named_data = flat_tensor_->named_data(); ET_CHECK_OR_RETURN_ERROR( - index < num_keys, + index < named_data->size(), InvalidArgument, "Index %" PRIu32 " out of range of size %" PRIu32, index, - num_keys); - const auto* named_data = flat_tensor_->named_data(); - ET_CHECK_OR_RETURN_ERROR( - named_data != nullptr, - InvalidExternalData, - "Named data is null"); + named_data->size()); const auto* entry = named_data->Get(index); ET_CHECK_OR_RETURN_ERROR( entry != nullptr && entry->key() != nullptr, InvalidExternalData, - "Named data entry or key at index %u is null", + "Named data entry or key at index %" PRIu32 + " is null. Malformed PTD file.", index); return entry->key()->c_str(); } diff --git a/extension/flat_tensor/test/flat_tensor_data_map_test.cpp b/extension/flat_tensor/test/flat_tensor_data_map_test.cpp index 1ba292014a4..512ffb40634 100644 --- a/extension/flat_tensor/test/flat_tensor_data_map_test.cpp +++ b/extension/flat_tensor/test/flat_tensor_data_map_test.cpp @@ -134,9 +134,6 @@ TEST_F(FlatTensorDataMapTest, GetKeys) { // Check get_key fails when out of bounds. Result key2_res = data_map->get_key(2); EXPECT_EQ(key2_res.error(), Error::InvalidArgument); - EXPECT_EQ( - data_map->get_key(std::numeric_limits::max()).error(), - Error::InvalidArgument); } TEST_F(FlatTensorDataMapTest, LoadInto) { @@ -211,7 +208,8 @@ struct SegmentSpec { // test loader, so large source offsets do not require a large allocation. std::vector CreateDataWithVersion( uint32_t version, - const SegmentSpec* segment = nullptr) { + const SegmentSpec* segment = nullptr, + bool with_null_key = false) { flatbuffers::FlatBufferBuilder builder; std::vector> @@ -226,13 +224,16 @@ std::vector CreateDataWithVersion( executorch_flatbuffer::ScalarType::FLOAT, builder.CreateVector(sizes), builder.CreateVector(dim_order)); - segments.push_back(flat_tensor_flatbuffer::CreateDataSegment( - builder, segment->offset, segment->size)); - named_data.push_back(flat_tensor_flatbuffer::CreateNamedData( - builder, - builder.CreateString(kWideTensorKey), - /*segment_index=*/0, - tensor_layout)); + segments.push_back( + flat_tensor_flatbuffer::CreateDataSegment( + builder, segment->offset, segment->size)); + named_data.push_back( + flat_tensor_flatbuffer::CreateNamedData( + builder, + with_null_key ? flatbuffers::Offset() + : builder.CreateString(kWideTensorKey), + /*segment_index=*/0, + tensor_layout)); } auto flat_tensor = flat_tensor_flatbuffer::CreateFlatTensor( @@ -387,6 +388,43 @@ TEST_F(FlatTensorDataMapTest, NewerSchemaVersionFailsToLoad) { EXPECT_EQ(data_map.error(), Error::InvalidExternalData); } +TEST_F(FlatTensorDataMapTest, KeylessNamedDataReturnsInvalidExternalData) { + constexpr SegmentSpec kSegment{0, sizeof(float), sizeof(float)}; + std::vector data = CreateDataWithVersion( + FlatTensorDataMap::kMaxSupportedSchemaVersion, + &kSegment, + /*with_null_key=*/true); + + alignas(std::max_align_t) std::array aligned_buffer{}; + ASSERT_LE(data.size(), aligned_buffer.size()); + std::memcpy(aligned_buffer.data(), data.data(), data.size()); + + Result header = + FlatTensorHeader::Parse(aligned_buffer.data(), data.size()); + ASSERT_TRUE(header.ok()); + const uint64_t absolute_offset = header->segment_base_offset; + const float segment_data = 1.0f; + WideOffsetDataLoader loader( + aligned_buffer.data(), + data.size(), + reinterpret_cast(&segment_data), + sizeof(segment_data), + absolute_offset + sizeof(segment_data)); + + Result data_map = FlatTensorDataMap::load(&loader); + ASSERT_TRUE(data_map.ok()); + + // get_key for the keyless entry must fail with InvalidExternalData. + EXPECT_EQ(data_map->get_key(0).error(), Error::InvalidExternalData); + + // Linear search in get_data and get_tensor_layout must encounter the keyless + // entry and fail with InvalidExternalData instead of silently skipping it. + EXPECT_EQ(data_map->get_data("any_key").error(), Error::InvalidExternalData); + EXPECT_EQ( + data_map->get_tensor_layout("any_key").error(), + Error::InvalidExternalData); +} + TEST_F(FlatTensorDataMapTest, RejectsOutOfBoundsRootOffset) { // A valid file loads. Result valid = data_map_loader_->load( diff --git a/runtime/executor/method.cpp b/runtime/executor/method.cpp index 65d4688b5de..1e53dd16ac6 100644 --- a/runtime/executor/method.cpp +++ b/runtime/executor/method.cpp @@ -1732,21 +1732,18 @@ Error Method::execute_instruction() { // checked at init time. auto op_index = instruction->instr_args_as_KernelCall()->op_index(); ET_UNUSED auto op = serialization_plan_->operators()->Get(op_index); - const char* const op_name = - (op != nullptr && op->name() != nullptr) ? op->name()->c_str() : "unknown"; - const bool has_overload = - op != nullptr && op->overload() != nullptr && op->overload()->size() > 0; - const char* const op_overload = - has_overload ? op->overload()->c_str() : ""; ET_LOG( Error, "KernelCall failed at instruction %" ET_PRIsize_t ":%" ET_PRIsize_t " in operator %s%s%s: 0x%x", step_state_.chain_idx, step_state_.instr_idx, - op_name, - has_overload ? "." : "", - op_overload, + op->name()->c_str(), + (op->overload() != nullptr && op->overload()->size() > 0) ? "." + : "", + (op->overload() != nullptr && op->overload()->size() > 0) + ? op->overload()->c_str() + : "", (unsigned int)err); for (size_t i = 0; i < args.size(); ++i) { ET_LOG( diff --git a/runtime/executor/test/CMakeLists.txt b/runtime/executor/test/CMakeLists.txt index 113884e980a..9501952179f 100644 --- a/runtime/executor/test/CMakeLists.txt +++ b/runtime/executor/test/CMakeLists.txt @@ -215,6 +215,19 @@ target_include_directories( "${EXECUTORCH_ROOT}/third-party/flatbuffers/include" ) +et_cxx_test( + program_validation_test SOURCES program_validation_test.cpp EXTRA_LIBS + extension_data_loader program_schema +) +add_dependencies(program_validation_test generated_pte_files) +set_property(TEST program_validation_test PROPERTY ENVIRONMENT ${test_env}) + +target_include_directories( + program_validation_test + PRIVATE "${CMAKE_INSTALL_PREFIX}/schema/include" + "${EXECUTORCH_ROOT}/third-party/flatbuffers/include" +) + et_cxx_test( kernel_resolution_test SOURCES kernel_resolution_test.cpp EXTRA_LIBS extension_data_loader diff --git a/runtime/executor/test/program_validation_test.cpp b/runtime/executor/test/program_validation_test.cpp index e6ef6246578..95e2f6505ac 100644 --- a/runtime/executor/test/program_validation_test.cpp +++ b/runtime/executor/test/program_validation_test.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -78,7 +79,8 @@ std::vector CreateTestProgram( const std::vector& input_indices = {}, // If true, emit a chain of two KernelCalls: the first's op_index is out of // range so resolve_operator() fails on a non-final instruction. - bool with_out_of_range_kernel_call = false) { + bool with_out_of_range_kernel_call = false, + bool with_failing_kernel_no_overload = false) { flatbuffers::FlatBufferBuilder builder(1024); std::vector> evalues; @@ -161,12 +163,33 @@ std::vector CreateTestProgram( /*outputs=*/builder.CreateVector(std::vector{}), builder.CreateVector(instructions), /*stacktrace=*/0)); + } else if (with_failing_kernel_no_overload) { + auto kernel_call = executorch_flatbuffer::CreateKernelCall( + builder, /*op_index=*/0, builder.CreateVector(std::vector{})); + std::vector> + instructions{executorch_flatbuffer::CreateInstruction( + builder, + executorch_flatbuffer::InstructionArguments::KernelCall, + kernel_call.Union())}; + chains.push_back( + executorch_flatbuffer::CreateChain( + builder, + /*inputs=*/builder.CreateVector(std::vector{}), + /*outputs=*/builder.CreateVector(std::vector{}), + builder.CreateVector(instructions), + /*stacktrace=*/0)); } auto chain_vec = builder.CreateVector(chains); std::vector> operators; if (with_out_of_range_kernel_call) { operators.push_back(executorch_flatbuffer::CreateOperator( builder, builder.CreateString("test::noop"), builder.CreateString(""))); + } else if (with_failing_kernel_no_overload) { + operators.push_back( + executorch_flatbuffer::CreateOperator( + builder, + builder.CreateString("test::failing_no_overload"), + /*overload=*/0)); } auto operators_vec = builder.CreateVector(operators); auto empty_delegates_vec = builder.CreateVector( @@ -189,6 +212,11 @@ std::vector CreateTestProgram( plans.push_back(execution_plan); auto plans_vec = builder.CreateVector(plans); + auto constant_segment = executorch_flatbuffer::CreateSubsegmentOffsets( + builder, + /*segment_index=*/0, + builder.CreateVector(std::vector{0})); + auto program = executorch_flatbuffer::CreateProgram( builder, /*version=*/0, @@ -196,7 +224,7 @@ std::vector CreateTestProgram( /*constant_buffer=*/0, /*backend_delegate_data=*/0, /*segments=*/0, - /*constant_segment=*/0, + constant_segment, /*mutable_data_segments=*/0, /*named_data=*/0); @@ -404,3 +432,36 @@ TEST_F(ProgramValidationTest, ResolveOperatorFailureSurfacesFromInit) { program->load_method("forward", &mmm.get()); EXPECT_EQ(method.error(), Error::InvalidProgram); } + +TEST_F(ProgramValidationTest, KernelCallFailureWithNullOverloadDoesNotCrash) { + static const auto fail_kernel = executorch::runtime::Kernel( + "test::failing_no_overload", + [](executorch::runtime::KernelRuntimeContext& context, + executorch::runtime::Span) { + context.fail(Error::Internal); + }); + ASSERT_EQ(executorch::runtime::register_kernel(fail_kernel), Error::Ok); + + AlignedBuffer buf(CreateTestProgram( + /*configs=*/{}, + /*input_indices=*/{}, + /*with_out_of_range_kernel_call=*/false, + /*with_failing_kernel_no_overload=*/true)); + auto loader = buf.loader(); + + Result program = + Program::load(&loader, Program::Verification::InternalConsistency); + ASSERT_EQ(program.error(), Error::Ok); + + executorch::runtime::testing::ManagedMemoryManager mmm( + /*planned_memory_bytes=*/32 * 1024U, + /*method_allocator_bytes=*/32 * 1024U); + Result method = + program->load_method("forward", &mmm.get()); + ASSERT_EQ(method.error(), Error::Ok); + + // KernelCall failure logs operator overload. With a null overload in the + // flatbuffer operator table, this previously segfaulted (#23955). + Error err = method->execute(); + EXPECT_EQ(err, Error::Internal); +}