Mkulakow/fixes

[michalkulakowski]

🛠 Summary

JIRA/Issue if applicable.
Describe the changes.

🧪 Checklist

• Unit tests added.
• The documentation updated.
• Change follows security best practices.
  ``

[Copilot]

Pull request overview

This PR hardens request-to-tensor conversion and custom node behaviors, primarily by adding stricter validation for mixed image batches and malformed KFS BYTES payloads, plus a fix to cap object-detection outputs to max_output_batch.

Changes:

• Reject mixed-channel image batches when converting native file inputs (and add coverage for ranged channel dimensions).
• Harden KFS BYTES/string parsing against overflow/wrap-style malformed length prefixes.
• Fix model_zoo_intel_object_detection to resize all output vectors consistently when capping to max_output_batch, and add a regression test.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/test/tensor_conversion_test.cpp	Adds negative tests for mixed-channel batches and malformed BYTES payloads (needs robust file-open/read checks).
src/test/node_library_manager_test.cpp	Adds regression test asserting object-detection node caps outputs to max_output_batch.
src/tensor_conversion.hpp	Adds mixed-channel-in-batch validation during native file input conversion.
src/tensor_conversion.cpp	Adds empty-batch guard and size-consistency check in createTensorFromMats.
src/tensor_conversion_common.cpp	Mirrors createTensorFromMats hardening in the common implementation.
src/kfs_frontend/kfs_utils.cpp	Reworks BYTES parsing to avoid accumulator overflow and out-of-bounds reads (uses memcpy + bounds checks).
src/filesystem/s3filesystem.cpp	Changes default scheme handling for custom S3 endpoints (potential behavior break for endpoints without an explicit scheme).
src/custom_nodes/model_zoo_intel_object_detection/model_zoo_intel_object_detection.cpp	Ensures all output vectors are resized consistently when capping results.
src/custom_nodes/common/custom_node_library_internal_manager.hpp	Fixes get_buffer() to avoid returning undersized queue buffers and avoids use of an uninitialized out-pointer.

Suppressed comments (1)

src/test/tensor_conversion_test.cpp:629

• Same issue here: the test assumes grayscale.jpg can be opened and read; on failure, tellg() may return -1 and the test will allocate an enormous buffer.

    std::ifstream grayscaleDataFile;
    grayscaleDataFile.open(getGenericFullPathForSrcTest("/ovms/src/test/binaryutils/grayscale.jpg"), std::ios::binary);
    grayscaleDataFile.seekg(0, std::ios::end);
    size_t grayscaleFilesize = grayscaleDataFile.tellg();
    grayscaleDataFile.seekg(0);

---

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

[rasapala]

Does it pass our minio validation ? Should we update documentation ?

[mzegla]

Should we simply break from those conditions? Not return error?

[rasapala]

Copilot had good suggestions.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/test/tensor_conversion_test.cpp:635

• This test reads grayscale.jpg without validating that the file opened successfully or that tellg()/read() succeeded. If the file is missing/corrupted, tellg() can return -1 which becomes a huge size_t, leading to an enormous allocation and undefined behavior during the test.

    std::ifstream grayscaleDataFile;
    grayscaleDataFile.open(getGenericFullPathForSrcTest("/ovms/src/test/binaryutils/grayscale.jpg"), std::ios::binary);
    grayscaleDataFile.seekg(0, std::ios::end);
    size_t grayscaleFilesize = grayscaleDataFile.tellg();
    grayscaleDataFile.seekg(0);
    std::unique_ptr<char[]> grayscaleImageBytes(new char[grayscaleFilesize]);
    grayscaleDataFile.read(grayscaleImageBytes.get(), grayscaleFilesize);

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

src/test/localfilesystem_test.cpp:62

• This S3 endpoint parsing test depends on ovms::S3FileSystem and Aws::Http::Scheme, so it will fail to compile when CLOUD_DISABLE=1 (cloud deps are not available). Wrap the test in #if CLOUD_DISABLE == 0 or move it to a cloud-only test translation unit.

TEST(S3FileSystem, ParseEndpointUsesHttpByDefault) {
    auto http_default = ovms::S3FileSystem::parseEndpoint("localhost:9000");
    EXPECT_EQ(http_default.first, "localhost:9000");
    EXPECT_EQ(http_default.second, Aws::Http::Scheme::HTTP);

src/filesystem/s3filesystem.cpp:63

• parseEndpoint() is documented as returning a clean host:port string, but it currently preserves a trailing / (e.g. https://localhost:9000/ -> localhost:9000/). Stripping trailing slashes makes the function behavior match the doc and avoids passing a non-host value to endpointOverride.

    }

    return {normalized, scheme};

src/test/tensor_conversion_test.cpp:200

• std::ifstream::read() takes a std::streamsize byte count. Passing size_t relies on an implicit narrowing conversion; use an explicit static_cast<std::streamsize> here (as done in the other new test below) to avoid warnings and keep the intent clear.

    ASSERT_TRUE(grayscaleDataFile.read(grayscaleImageBytes.get(), grayscaleFilesize).good());

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/test/tensor_conversion_test.cpp:200

• std::ifstream::read expects a std::streamsize length. Passing size_t relies on an implicit signed conversion and can cause warnings or incorrect reads on platforms where size_t is wider than std::streamsize.

Cast grayscaleFilesize to std::streamsize (as done in the other test in this file).

    std::unique_ptr<char[]> grayscaleImageBytes(new char[grayscaleFilesize]);
    ASSERT_TRUE(grayscaleDataFile.read(grayscaleImageBytes.get(), grayscaleFilesize).good());

src/test/node_library_manager_test.cpp:132

• After library.initialize(...) succeeds, the test immediately passes customNodeLibraryInternalManager into execute(...). Add an assertion that initialize actually set the pointer to a non-null value, so failures are reported clearly (instead of a potential null deref inside the library).

    void* customNodeLibraryInternalManager = nullptr;
    ASSERT_EQ(library.initialize(&customNodeLibraryInternalManager, params.data(), params.size()), 0);