Conversation
atobiszei
force-pushed
the
atobisze_improve_img_validation
branch
from
September 9, 2026 09:54
088e6a0 to
2d24293
Compare
atobiszei
force-pushed
the
atobisze_improve_img_validation
branch
from
September 14, 2026 09:31
e765170 to
d0a9be7
Compare
atobiszei
marked this pull request as ready for review
September 14, 2026 09:31
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Critical validation bypasses and aggregate-limit issues remain, along with test and API compatibility problems.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds decoded-image size estimation and configurable pixel-budget validation across image-processing paths.
Changes:
- Adds image validation, configuration, status mappings, and container defaults.
- Integrates checks across tensor, multimodal, and image-generation inputs.
- Updates utilities, tests, documentation, and build targets.
File summaries
| File | Reviewed changes / final finding |
|---|---|
src/test/text2image_test.cpp |
Updates image conversion include. |
src/test/test_utils.hpp |
Adds scoped configuration restoration. |
src/test/tensor_conversion_test.cpp |
Tests image pixel budgets. |
src/test/predict_validation_test.cpp |
Adds validation tests; moderate (2 votes): fixes needed for unsafe uint32_t* writes. |
src/test/decoded_image_size_test.cpp |
Adds estimator tests; moderate (3 votes): referenced image fixtures are missing, causing test failures. |
src/tensor_conversion.hpp |
Adds image checks; critical (3 votes): per-request aggregate pixel limits can be bypassed across multiple tensors. |
src/tensor_conversion_common.hpp |
Declares budget helpers. |
src/tensor_conversion_common.cpp |
Implements image budget enforcement. |
src/status.hpp |
Adds image-size status; critical (1 vote): insertion renumbers existing C API status values. |
src/status.cpp |
Adds status message; nit (3 votes): wording incorrectly describes a payload limit. |
src/server.cpp |
Adds an unused header; nit (3 votes). |
src/predict_request_validation_utils_impl.hpp |
Declares image configuration accessors. |
src/predict_request_validation_utils_impl.cpp |
Implements accessors; nit (1 vote): removes an unused stringutils.hpp dependency. |
src/mediapipe_internal/mediapipegraphconfig.cpp |
Updates status include path. |
src/llm/io_processing/input_processors/image_decoding_processor.cpp |
Uses the renamed image-fetch API. |
src/llm/io_processing/image_utils.hpp |
Renames image-loading API. |
src/llm/io_processing/image_utils.cpp |
Adds validation; critical (3 votes): local-file decoding bypasses the pixel check. |
src/llm/BUILD |
Updates image utility dependencies. |
src/image_utils/image_conversion.hpp |
Adds relocated conversion interface. |
src/image_utils/image_conversion.cpp |
Relocates conversion implementation. |
src/image_utils/decoded_image_size.hpp |
Defines size-estimation API. |
src/image_utils/decoded_image_size.cpp |
Implements image header parsing. |
src/image_utils/BUILD |
Adds image utility targets. |
src/image_gen/imagegenutils.cpp |
Updates conversion include. |
src/image_gen/http_image_gen_calculator.cc |
Updates conversion include; critical (1 vote): image-edit and inpainting uploads bypass the guard for image and mask decodes. |
src/image_gen/BUILD |
Updates image conversion labels. |
src/http_server.cpp |
Maps image-size errors to HTTP errors. |
src/grpc_utils.cpp |
Maps image-size errors to gRPC errors. |
src/config.hpp |
Adds image setting accessors. |
src/config.cpp |
Implements image setting accessors. |
src/cli_parser.cpp |
Adds image validation CLI options. |
src/capi_frontend/server_settings.hpp |
Adds image validation settings. |
src/BUILD |
Updates dependencies and test registration. |
docs/security_considerations.md |
Documents decompression protection. |
docs/parameters.md |
Documents image validation parameters. |
Dockerfile.ubuntu |
Sets the OpenCV pixel-limit default. |
Dockerfile.redhat |
Sets the OpenCV pixel-limit default. |
Review details
Suppressed comments (2)
src/llm/io_processing/image_utils.cpp:197
- This check is applied independently to each image.
ImageDecodingProcessorloops over allimage_urlparts and retains every decoded tensor inreq.inputImages, so one request can consume N times the configured pixel budget. If the documented per-request limit is intended, keep a running total in the request/processor and check it before each decode.
if (estimate == image_utils::DecodedSizeEstimate::Estimated &&
estimatedDecodedPixels > request_validation_utils::getMaxImageDecodePixels()) {
src/predict_request_validation_utils_impl.cpp:26
stringutils.hppis newly included but no symbol from it is used in this translation unit. Please remove the include so this file does not add an unnecessary direct dependency and build cost.
#include "stringutils.hpp"
- Files reviewed: 36/37 changed files
- Comments generated: 8
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| #include "../image_conversion.hpp" | ||
| #include "src/http_payload.hpp" | ||
| #include "src/logging.hpp" | ||
| #include "src/image_utils/image_conversion.hpp" |
| } | ||
| try { | ||
| tensor = loadImageStbiFromFile(resolvedImagePathStr.c_str()); | ||
| return loadImageStbiFromFile(resolvedImagePathStr.c_str()); |
| INVALID_STRING_INPUT, /*!< Invalid string input */ | ||
| INVALID_INPUT_FORMAT, /*!< Invalid format of the input inside buffer */ | ||
| INVALID_STRING_MAX_SIZE_EXCEEDED, /*!< Maximum 2D array after string conversion exceeded 1GB */ | ||
| INVALID_IMAGE_MAX_SIZE_EXCEEDED, /*!< Image binary payload size exceeds maximum allowed limit */ |
Comment on lines
+92
to
+93
| size_t totalAllocatedPixels = 0; | ||
| size_t maxAllowedImagePixels = request_validation_utils::getMaxImageDecodePixels(); |
Comment on lines
+122
to
+123
| const std::string data = readImageFixture(c.file); | ||
| ASSERT_FALSE(data.empty()) << "fixture missing: " << c.file; |
|
|
||
| const uint32_t payloadLen = 2 * 1024 * 1024; | ||
| std::string largeRawBuffer(sizeof(uint32_t) + payloadLen, 'A'); | ||
| *reinterpret_cast<uint32_t*>(&largeRawBuffer[0]) = payloadLen; |
| {StatusCode::INVALID_DEVICE_ID, "Invalid input buffer device id"}, | ||
| {StatusCode::INVALID_STRING_INPUT, "Invalid string input"}, | ||
| {StatusCode::INVALID_STRING_MAX_SIZE_EXCEEDED, "Maximum 2D array after string conversion exceeded 1GB"}, | ||
| {StatusCode::INVALID_IMAGE_MAX_SIZE_EXCEEDED, "Image binary payload size exceeds maximum allowed limit"}, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Improve OVMS validation by adding pre-decode image pixels size checks.
JIRA:CVS-194289