Copilot commented on code in PR #2251:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2251#discussion_r3903143036
##########
core-framework/common/src/io/InputStream.cpp:
##########
@@ -47,44 +49,51 @@ size_t InputStream::read(utils::Identifier &value) {
return ret;
}
-size_t InputStream::read(std::string &str, bool widen) {
+size_t InputStream::read(std::string &str, LengthPrefixSize prefix_size,
size_t max_length) {
uint32_t string_length = 0;
- size_t length_return = 0;
- if (!widen) {
- uint16_t shortLength = 0;
- length_return = read(shortLength);
- string_length = shortLength;
- } else {
- length_return = read(string_length);
+ size_t length_prefix_size_in_bytes = 0;
+ switch (prefix_size) {
+ case LengthPrefixSize::_16BIT: {
+ uint16_t out_16bit_length = 0;
+ length_prefix_size_in_bytes = read(out_16bit_length);
+ string_length = out_16bit_length;
+ } break;
+ case LengthPrefixSize::_32BIT: {
+ uint32_t out_32bit_length = 0;
+ length_prefix_size_in_bytes = read(out_32bit_length);
+ string_length = out_32bit_length;
+ } break;
}
- if (length_return == 0 || isError(length_return)) {
- return length_return;
+ // The zero case should be impossible, read(Integral&) returns the size of
the integral type,
+ // or an error code, so this handles the error case and propagates the
stream error
+ if (length_prefix_size_in_bytes == 0 ||
isError(length_prefix_size_in_bytes)) {
+ return length_prefix_size_in_bytes;
}
+ string_length = std::min(string_length, gsl::narrow<uint32_t>(max_length));
if (string_length == 0) {
str.clear();
- return length_return;
+ return length_prefix_size_in_bytes;
}
Review Comment:
Clamping `string_length` to `max_length` changes how many bytes are consumed
from the stream: if the incoming prefix advertises a larger length than
`max_length`, this code will only read `max_length` bytes and leave the
remainder in the stream, desynchronizing subsequent reads (and potentially
causing protocol parsing failures). Prefer returning an error (and/or setting
stream error state) when `advertised_length > max_length`; alternatively,
explicitly consume and discard the remaining bytes to keep the stream position
consistent.
##########
minifi-api/common/include/minifi-cpp/io/InputStream.h:
##########
@@ -40,11 +45,13 @@ class InputStream : public virtual Stream {
virtual size_t read(std::span<std::byte> out_buffer) = 0;
/**
- * Read string from stream. Use isError (Stream.h) to check for errors.
+ * Read length prefixed string from stream. Use isError (Stream.h) to check
for errors.
* @param str reference string
+ * @param length_prefix_size The wideness of the length prefix, 16bit or
32bit
+ * @param max_length The max length of the string, to avoid excessive
allocations
* @return resulting read size or STREAM_ERROR on error or
static_cast<size_t>(-2) on EAGAIN
**/
- size_t read(std::string &str, bool widen = false);
+ size_t read(std::string &str, LengthPrefixSize length_prefix_size, size_t
max_length);
Review Comment:
This changes a public header API from `read(std::string&, bool widen=false)`
to a required-args overload, which is a breaking change for external consumers
of `minifi-api`. Consider adding an overload that preserves the old signature
(possibly marked deprecated) and forwards to the new implementation with
appropriate defaults (e.g., `_16BIT` and a conservative max), so downstream
code can migrate without immediate breakage.
##########
libminifi/src/provenance/Provenance.cpp:
##########
@@ -339,14 +341,16 @@ bool
ProvenanceEventRecordImpl::deserialize(io::InputStream &input_stream) {
for (uint32_t i = 0; i < numAttributes; i++) {
std::string key;
{
- const auto ret = input_stream.read(key);
+ // clamp attribute name / value to 64k (the 16bit length prefix maximum)
+ const auto ret = input_stream.read(key, io::LengthPrefixSize::_16BIT,
64_KiB);
Review Comment:
The comment states the 16-bit length prefix maximum, but `64_KiB` is 65536
while a 16-bit unsigned maximum is 65535. Either adjust the limit to
`std::numeric_limits<uint16_t>::max()` (or `64_KiB - 1`) or update the comment
to avoid an off-by-one mismatch.
##########
core-framework/common/src/io/InputStream.cpp:
##########
@@ -47,44 +49,51 @@ size_t InputStream::read(utils::Identifier &value) {
return ret;
}
-size_t InputStream::read(std::string &str, bool widen) {
+size_t InputStream::read(std::string &str, LengthPrefixSize prefix_size,
size_t max_length) {
uint32_t string_length = 0;
- size_t length_return = 0;
- if (!widen) {
- uint16_t shortLength = 0;
- length_return = read(shortLength);
- string_length = shortLength;
- } else {
- length_return = read(string_length);
+ size_t length_prefix_size_in_bytes = 0;
+ switch (prefix_size) {
+ case LengthPrefixSize::_16BIT: {
+ uint16_t out_16bit_length = 0;
+ length_prefix_size_in_bytes = read(out_16bit_length);
+ string_length = out_16bit_length;
+ } break;
+ case LengthPrefixSize::_32BIT: {
+ uint32_t out_32bit_length = 0;
+ length_prefix_size_in_bytes = read(out_32bit_length);
+ string_length = out_32bit_length;
+ } break;
}
- if (length_return == 0 || isError(length_return)) {
- return length_return;
+ // The zero case should be impossible, read(Integral&) returns the size of
the integral type,
+ // or an error code, so this handles the error case and propagates the
stream error
+ if (length_prefix_size_in_bytes == 0 ||
isError(length_prefix_size_in_bytes)) {
+ return length_prefix_size_in_bytes;
}
+ string_length = std::min(string_length, gsl::narrow<uint32_t>(max_length));
if (string_length == 0) {
str.clear();
- return length_return;
+ return length_prefix_size_in_bytes;
}
str.clear();
- str.reserve(string_length);
+ str.resize(string_length);
+ std::span<std::byte> dst_buffer = as_writable_bytes(std::span{str});
Review Comment:
`std::span{str}` is not a standard C++ way to create a `std::span` from
`std::string` (there is no `std::span` deduction guide/constructor for
`basic_string`), so this may fail to compile depending on toolchain/standard
library. Construct the span explicitly from `str.data()` and `str.size()` (as a
`span<char>` or `span<std::byte>` via a safe cast helper) before calling
`as_writable_bytes`.
##########
libminifi/src/provenance/Provenance.cpp:
##########
@@ -339,14 +341,16 @@ bool
ProvenanceEventRecordImpl::deserialize(io::InputStream &input_stream) {
for (uint32_t i = 0; i < numAttributes; i++) {
std::string key;
{
- const auto ret = input_stream.read(key);
+ // clamp attribute name / value to 64k (the 16bit length prefix maximum)
+ const auto ret = input_stream.read(key, io::LengthPrefixSize::_16BIT,
64_KiB);
if (ret == 0 || io::isError(ret)) {
return false;
}
}
std::string value;
{
- const auto ret = input_stream.read(value);
+ // clamp attribute name / value to 64k (the 16bit length prefix maximum)
+ const auto ret = input_stream.read(value, io::LengthPrefixSize::_16BIT,
64_KiB);
Review Comment:
The comment states the 16-bit length prefix maximum, but `64_KiB` is 65536
while a 16-bit unsigned maximum is 65535. Either adjust the limit to
`std::numeric_limits<uint16_t>::max()` (or `64_KiB - 1`) or update the comment
to avoid an off-by-one mismatch.
##########
core-framework/common/src/io/InputStream.cpp:
##########
@@ -47,44 +49,51 @@ size_t InputStream::read(utils::Identifier &value) {
return ret;
}
-size_t InputStream::read(std::string &str, bool widen) {
+size_t InputStream::read(std::string &str, LengthPrefixSize prefix_size,
size_t max_length) {
Review Comment:
The new max-length behavior should be covered with a unit test that encodes
an advertised length larger than `max_length` and asserts the expected outcome
(error vs. truncation + discard), including verifying that subsequent reads
remain correctly aligned. Adding such a test would prevent regressions in
stream framing and protocol parsing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
