Reorder MP checks and account for MP types

[mzegla]

No description provided.

[Copilot]

🟡 Changes recommended

A critical validation-order issue remains, along with required test and teardown fixes.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds MediaPipe tensor shape and byte-size validation, with regression coverage for oversized requests.

Changes:

• Adds dimension and buffer-size guards.
• Adds oversized-shape and server-survival tests.

File summaries

File	Changes and review findings
src/kfs_frontend/kfs_graph_executor_impl.cpp	Adds tensor validation. Critical (2 votes, line 416): content validation still occurs after tensor construction, allowing oversized allocations. Nit (1 vote, line 397): add coverage for dimensions above INT_MAX.
src/test/mediapipeflow_test.cpp	Adds oversized-shape regression coverage. Moderate (1 vote, line 3875): stop and join the server before teardown. Nits (1 vote, line 3938; 2 votes, line 3934): assert INVALID_ARGUMENT and separately cover INT_MAX + 1.

Review details

Suppressed comments (3)

src/kfs_frontend/kfs_graph_executor_impl.cpp:403

• The new narrowing guard for dimensions above INT_MAX is not exercised by the regression test: it sends exactly INT_MAX at line 3934, which bypasses this branch and is rejected only by the later byte-size check. Add a separate request with INT_MAX + 1 and assert INVALID_ARGUMENT so this conversion-safety path is covered while retaining the current INT_MAX case.

            if (requestInputItr->shape()[i] > static_cast<int64_t>(std::numeric_limits<int>::max())) {
                std::stringstream ss;
                ss << "Dimension size too large for Mediapipe tensor: " << tensorShapeToString(requestInputItr->shape()) << "; input name: " << requestedName;
                const std::string details = ss.str();
                SPDLOG_DEBUG("[servable name: {} version: {}] Invalid shape - {}", request.model_name(), request.model_version(), details);
                return Status(StatusCode::INVALID_SHAPE, details);
            }

src/test/mediapipeflow_test.cpp:3938

• EXPECT_NE accepts any non-OK failure, including UNKNOWN_ERROR from an allocation exception or a different validation path, so this regression test can pass without proving the new oversized-buffer guard. Assert grpc::StatusCode::INVALID_ARGUMENT, which is the gRPC mapping for the INVALID_CONTENT_SIZE returned by this path.

    EXPECT_NE(maliciousStatus.error_code(), grpc::StatusCode::OK);

src/test/mediapipeflow_test.cpp:3878

• TestWithTempDir::TearDown() removes the config/graph directory while the server thread is still running. If the server is still reading or holding these files (especially on Windows), cleanup can race or fail before stopServer()/join() completes; stop and join the server first, then invoke the base teardown.

        TestWithTempDir::TearDown();
        stopServer();
        t->join();
    }

• Files reviewed: 2/2 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Address the teardown-order issue and add the requested allocation-overflow and raw-input regression coverage.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

src/kfs_frontend/kfs_graph_executor_impl.cpp:411

• Could you add a regression case that uses a valid int dimension whose FP32 buffer size is just over INT_MAX (for example, 536870912) while providing a small payload? The new tests exercise dimensions above the int limit and a below-limit content mismatch, but do not execute this expectedBytes > INT_MAX branch, so a regression in the allocation-overflow guard would go unnoticed.

        if (!expectedBufferSizeValid || expectedBytes > static_cast<size_t>(std::numeric_limits<int>::max())) {

src/kfs_frontend/kfs_graph_executor_impl.cpp:421

• The new pre-allocation validation has separate inline-content and raw_input_contents branches, but both regression tests populate input.contents(). Please add a case with a mismatched raw buffer and the large declared shape; otherwise a regression that moves validateRawInputContent back after mediapipe::Tensor construction could reintroduce the multi-GB allocation for the raw-input path without being detected.

        if (request.raw_input_contents().size()) {
            OVMS_RETURN_ON_FAIL(validateRawInputContent(expectedBytes, request.raw_input_contents().at(inputIndex), requestedName, request));
        } else {
            OVMS_RETURN_ON_FAIL(validateInputContent(*requestInputItr, expectedBytes, requestedName, request));

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite