fix: Windows large file support and binary-safe downloads - #248
Open
jiuker wants to merge 1 commit into
Open
Conversation
Fixes two Windows (LLP64) issues reported via the VCPKG port: binary file corruption on download and 32-bit 'long' overflow for files larger than 2 GiB. Binary-safe downloads: - DownloadObject: open the temp output stream with std::ios::binary. On Windows the default text mode translates newlines and treats 0x1A as EOF, corrupting downloaded binary content (e.g. zip archives). Large file support (LLP64: 'long' is strictly 32-bit on Windows): - SelectResult (types.h) and the parser in select.cc: bytes_scanned / bytes_processed / bytes_returned switch from 'long int' to 'long long', and std::stol to std::stoll - StatObject (baseclient.cc): parse the content-length header with std::stoll so resp.size (size_t) is not truncated above 2 GiB - ComposeSource (args.h/args.cc): object_size_ member and its assignment switch from 'long' to 'long long' so composing sources larger than 2 GiB does not overflow No behavioral change on LP64 platforms (Linux/macOS), where 'long' is already 64-bit. Files changed: include/miniocpp/args.h, include/miniocpp/types.h, src/args.cc, src/baseclient.cc, src/client.cc, src/select.cc
📝 WalkthroughWalkthroughThe change improves Windows large-file compatibility by using ChangesLarge-file and binary compatibility
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@include/miniocpp/args.h`:
- Line 385: Replace ComposeSource::object_size_’s -1 sentinel with
std::optional<size_t>, initialize it empty, and set it when BuildHeaders
succeeds. Update ComposeSource::ObjectSize() to check has_value() and return the
stored size through the existing size_t API without signed-to-unsigned
conversion.
In `@include/miniocpp/types.h`:
- Around line 296-298: Update include/miniocpp/types.h at lines 296-298 and
305-309 to include <optional> and change the three public Select metric fields
and corresponding constructor parameters to std::optional<long long>. Update
src/select.cc lines 198-212 to leave each metric unset when its XML text is
empty and assign the parsed long long value otherwise.
In `@src/select.cc`:
- Around line 198-212: Add focused test coverage for SelectHandler::DataFunction
by supplying a synthetic Stats event whose BytesScanned, BytesProcessed, and
BytesReturned values all exceed INT32_MAX, then assert the callback receives the
exact long long values. Keep the test independent of large objects and ensure it
would detect any regression from std::stoll to std::stol, particularly on
Windows.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ced86126-1a6c-4cd9-9d7c-2a8a831768b9
📒 Files selected for processing (6)
include/miniocpp/args.hinclude/miniocpp/types.hsrc/args.ccsrc/baseclient.ccsrc/client.ccsrc/select.cc
|
|
||
| private: | ||
| long object_size_ = -1; | ||
| long long object_size_ = -1; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files 'include/miniocpp/args.h' 'include/miniocpp/*.h' | head -50
printf '%s\n' '--- relevant symbols ---'
rg -n -C 8 'object_size_|ObjectSize\(|BuildHeaders|ComposeSource' include/miniocpp/args.h
printf '%s\n' '--- optional usage and includes ---'
rg -n '`#include` <optional>|std::optional|size_t|long long' include/miniocpp/args.h include/miniocppRepository: minio/minio-cpp
Length of output: 49262
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- all ComposeSource references ---'
rg -n -C 12 'ComposeSource::(BuildHeaders|ObjectSize|Headers)|\.ObjectSize\(\)|\.BuildHeaders\(' --glob '*.{h,cc,cpp,hpp}' .
printf '%s\n' '--- declaration context ---'
sed -n '1,35p' include/miniocpp/args.h
sed -n '376,390p' include/miniocpp/args.hRepository: minio/minio-cpp
Length of output: 6873
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- ComposeSource implementation ---'
sed -n '350,415p' src/args.cc
printf '%s\n' '--- ComposeObject call path ---'
sed -n '450,515p' src/client.cc
sed -n '575,620p' src/client.cc
printf '%s\n' '--- direct object_size_ accesses ---'
rg -n -C 3 'object_size_' --glob '*.{h,cc,cpp,hpp}' .
printf '%s\n' '--- read-only structural verifier ---'
python3 - <<'PY'
from pathlib import Path
header = Path("include/miniocpp/args.h").read_text()
impl = Path("src/args.cc").read_text()
client = Path("src/client.cc").read_text()
assert "long long object_size_ = -1;" in header
assert "error::Error BuildHeaders(size_t object_size" in header
assert "size_t ComposeSource::ObjectSize() const" in impl
assert "object_size_ = static_cast<long long>(object_size);" in impl
assert "if (object_size_ == -1)" in impl
assert "return object_size_;" in impl
assert "source.BuildHeaders(size, etag)" in client
assert "size_t size = source.ObjectSize();" in client
print("The current representation is a signed sentinel, BuildHeaders receives size_t, and ObjectSize returns size_t.")
print("BuildHeaders is called before ObjectSize in the compose-object path.")
PYRepository: minio/minio-cpp
Length of output: 7204
Replace the sentinel with std::optional.
object_size_ uses -1 until ComposeSource::BuildHeaders succeeds. Use std::optional<size_t> and check has_value() in ComposeSource::ObjectSize(). This matches the size_t API and removes the signed-to-unsigned conversion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@include/miniocpp/args.h` at line 385, Replace ComposeSource::object_size_’s
-1 sentinel with std::optional<size_t>, initialize it empty, and set it when
BuildHeaders succeeds. Update ComposeSource::ObjectSize() to check has_value()
and return the stored size through the existing size_t API without
signed-to-unsigned conversion.
Source: Coding guidelines
Comment on lines
+296
to
+298
| long long bytes_scanned = -1; | ||
| long long bytes_processed = -1; | ||
| long long bytes_returned = -1; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- relevant declarations and parser ---'
sed -n '260,330p' include/miniocpp/types.h
sed -n '170,230p' src/select.cc
printf '%s\n' '--- all metric references ---'
rg -n --glob '*.{h,cc,cpp,hpp}' 'bytes_(scanned|processed|returned)|SelectResult' .
printf '%s\n' '--- standard and include context ---'
sed -n '1,80p' include/miniocpp/types.h
rg -n 'cxx_std|CXX_STANDARD|std=c\\+\\+|optional' CMakeLists.txt cmake include src test tests 2>/dev/null || trueRepository: minio/minio-cpp
Length of output: 12649
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- C++ standard configuration and support text ---'
sed -n '130,185p' CMakeLists.txt
rg -n -i 'c\\+\\+([[:space:]]|_)?(11|14|17|20)|MINIO_CPP_STD|standard' README.md CMakeLists.txt cmake include src tests examples 2>/dev/null | head -120
printf '%s\n' '--- optional include conventions ---'
sed -n '1,45p' include/miniocpp/args.h
sed -n '1,45p' include/miniocpp/utils.h
printf '%s\n' '--- all direct metric consumers and SelectResult construction context ---'
rg -n -C 4 'bytes_(scanned|processed|returned)|SelectResult\\(' examples tests src include
printf '%s\n' '--- select handler declarations and reset paths ---'
sed -n '1,190p' src/select.cc
sed -n '1,90p' include/miniocpp/select.h
printf '%s\n' '--- tracked project files mentioning SelectResult metrics ---'
git ls-files | rg '(^|/)(CMakeLists\\.txt|README|.*\\.(h|hpp|cc|cpp))$' | xargs rg -n 'bytes_(scanned|processed|returned)|SelectResult' 2>/dev/null || trueRepository: minio/minio-cpp
Length of output: 5998
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
types = Path("include/miniocpp/types.h").read_text()
select = Path("src/select.cc").read_text()
field_names = ["bytes_scanned", "bytes_processed", "bytes_returned"]
print("field_sentinel_count:")
for name in field_names:
print(name, len(re.findall(rf"\b{name}\s*=\s*-1\b", types)))
print("parser_sentinel_count:")
for name in field_names:
print(name, len(re.findall(rf"\blong long\s+{name}\s*=\s*-1\b", select)))
print("parser_assignment_conditions:")
for name in field_names:
match = re.search(
rf"long long\s+{name}\s*=\s*-1;(?P<body>.*?)(?=long long|\n\s*cont\s*=)",
select,
re.S,
)
body = match.group("body") if match else ""
print(name, "empty_value_keeps_sentinel=" + str("if (!value.empty())" in body))
print("public_select_result_constructors:")
for match in re.finditer(r"SelectResult\([^)]*\)", types):
print(" ", " ".join(match.group(0).split()))
PYRepository: minio/minio-cpp
Length of output: 704
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- direct references ---'
rg -n -F -e 'bytes_scanned' -e 'bytes_processed' -e 'bytes_returned' -e 'SelectResult(' examples tests src include
printf '%s\n' '--- SelectResult API and parser with line numbers ---'
cat -n include/miniocpp/types.h | sed -n '288,318p'
cat -n src/select.cc | sed -n '180,220p'
printf '%s\n' '--- Select-related documentation and protocol fixtures ---'
rg -n -i -F -e 'BytesScanned' -e 'BytesProcessed' -e 'BytesReturned' -e 'SelectObjectContent' -e 'SelectResult' . --glob '!include/miniocpp/cuda.h' --glob '!build/**'
printf '%s\n' '--- all constructors and result paths in select.cc ---'
rg -n -C 3 -F 'SelectResult(' src/select.ccRepository: minio/minio-cpp
Length of output: 11956
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
types = Path("include/miniocpp/types.h").read_text()
select = Path("src/select.cc").read_text()
names = ("bytes_scanned", "bytes_processed", "bytes_returned")
print("standard_declared_by_cmake:")
cmake = Path("CMakeLists.txt").read_text()
print(re.search(r'set\(MINIO_CPP_STD "([^"]+)"', cmake).group(1))
print("public_field_initializers:")
for name in names:
print(name, bool(re.search(rf'^\s*long long\s+{name}\s*=\s*-1;\s*$', types, re.M)))
print("parser_local_initializers:")
for name in names:
print(name, bool(re.search(rf'^\s*long long\s+{name}\s*=\s*-1;\s*$', select, re.M)))
print("parser_nonempty_assignments:")
for name in names:
pattern = rf'if\s*\(!value\.empty\(\)\)\s*{name}\s*=\s*std::stoll\(value\);'
print(name, bool(re.search(pattern, select)))
block = re.search(
r'\s*long long bytes_scanned = -1;.*?'
r'(?=\n\s*cont = result_func_\()',
select, re.S
).group(0)
print("parser_block_preserves_absence_for_empty_text:")
print(all(f'if (!value.empty()) {name} = std::stoll(value);' in block for name in names))
print("metric_reads_outside_declaration_and_parser:")
for path in Path(".").rglob("*"):
if not path.is_file() or any(part in {"build", ".git"} for part in path.parts):
continue
try:
text = path.read_text()
except UnicodeDecodeError:
continue
for name in names:
for match in re.finditer(rf'\b{re.escape(name)}\b', text):
line = text.count("\n", 0, match.start()) + 1
if not ((path.as_posix() == "include/miniocpp/types.h" and 293 <= line <= 315)
or (path.as_posix() == "src/select.cc" and 198 <= line <= 215)):
print(path, line, name)
PYRepository: minio/minio-cpp
Length of output: 544
Use std::optional for missing Select metrics.
Use std::optional<long long> for the three public fields and constructor parameters. Include <optional> in include/miniocpp/types.h. In src/select.cc, leave metrics unset when XML text is empty and assign parsed values otherwise.
📍 Affects 2 files
include/miniocpp/types.h#L296-L298(this comment)include/miniocpp/types.h#L305-L309src/select.cc#L198-L212
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@include/miniocpp/types.h` around lines 296 - 298, Update
include/miniocpp/types.h at lines 296-298 and 305-309 to include <optional> and
change the three public Select metric fields and corresponding constructor
parameters to std::optional<long long>. Update src/select.cc lines 198-212 to
leave each metric unset when its XML text is empty and assign the parsed long
long value otherwise.
Source: Coding guidelines
Comment on lines
+198
to
+212
| long long bytes_scanned = -1; | ||
| long long bytes_processed = -1; | ||
| long long bytes_returned = -1; | ||
|
|
||
| text = root.node().select_node("BytesScanned/text()"); | ||
| value = text.node().value(); | ||
| if (!value.empty()) bytes_scanned = std::stol(value); | ||
| if (!value.empty()) bytes_scanned = std::stoll(value); | ||
|
|
||
| text = root.node().select_node("BytesProcessed/text()"); | ||
| value = text.node().value(); | ||
| if (!value.empty()) bytes_processed = std::stol(value); | ||
| if (!value.empty()) bytes_processed = std::stoll(value); | ||
|
|
||
| text = root.node().select_node("BytesReturned/text()"); | ||
| value = text.node().value(); | ||
| if (!value.empty()) bytes_returned = std::stol(value); | ||
| if (!value.empty()) bytes_returned = std::stoll(value); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'BytesScanned|BytesProcessed|BytesReturned|bytes_scanned|bytes_processed|bytes_returned' tests src include || trueRepository: minio/minio-cpp
Length of output: 2771
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Select test section ---'
sed -n '540,680p' tests/tests.cc
printf '%s\n' '--- SelectResult and Select callbacks ---'
rg -n -C 5 'SelectResult|select.*callback|bytes_scanned|bytes_processed|bytes_returned' tests src include
printf '%s\n' '--- Select-related test fixtures and request setup ---'
rg -n -C 5 'select|Select|CSV|JSON|Parquet|BytesScanned|BytesProcessed|BytesReturned' tests | head -n 400Repository: minio/minio-cpp
Length of output: 23171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- SelectHandler structure and parser ---'
ast-grep outline src/select.cc
sed -n '1,270p' src/select.cc
printf '%s\n' '--- Select-related declarations ---'
sed -n '1,110p' include/miniocpp/select.h
sed -n '285,320p' include/miniocpp/types.h
printf '%s\n' '--- Test and build files ---'
git ls-files | rg '(^|/)(CMakeLists\.txt|Makefile|meson\.build|tests?|test|select)' | head -n 200
rg -n -C 3 'add_executable|enable_testing|gtest|SelectHandler|DataFunction|http::DataFunctionArgs' CMakeLists.txt tests include src 2>/dev/null | head -n 300Repository: minio/minio-cpp
Length of output: 22017
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Platform and compiler configuration ---'
git ls-files '.github' 'CMakeLists.txt' '*.yml' '*.yaml' '*.json' | head -n 200
rg -n -C 3 'windows|Windows|msvc|MSVC|clang-cl|Visual Studio|CXX_STANDARD|MINIO_CPP_STD' .github CMakeLists.txt 2>/dev/null | head -n 300Repository: minio/minio-cpp
Length of output: 5820
🌐 Web query:
Microsoft C++ std::stol std::stoll return type long long Windows LLP64 long range documentation
💡 Result:
In the Microsoft C++ (MSVC) implementation, the return types for the standard library string conversion functions are as follows: - std::stol: returns a long [1][2]. - std::stoll: returns a long long [1][3]. Under the Windows LLP64 data model (used by 64-bit Windows), these types maintain the following widths: - long: 32 bits [4][5][6]. Despite the name, on Windows, the long type remains 32 bits wide, consistent with its size in 32-bit Windows [4][5]. - long long: 64 bits [5][7][6]. This is a 64-bit signed integer, guaranteed to be at least 64 bits wide by the C++ standard [6]. Because std::stol returns a long, its effective range on Windows is restricted to the 32-bit range (approximately ±2.14 billion), even on 64-bit systems [8][7]. In contrast, std::stoll provides the full 64-bit range (approximately ±9.22 quintillion) [7]. If a numeric string exceeds the range of the return type (e.g., passing a 64-bit integer string to std::stol), the function will throw a std::out_of_range exception [1][2][9].
Citations:
- 1: https://en.cppreference.com/cpp/string/basic_string/stol
- 2: https://learn.microsoft.com/en-us/cpp/standard-library/string-functions?view=msvc-170
- 3: https://cplusplus.com/reference/string/stoll/
- 4: https://learn.microsoft.com/en-us/windows/win32/winprog64/abstract-data-models
- 5: https://learn.microsoft.com/en-us/previous-versions/tn-archive/bb496995(v=technet.10)
- 6: https://cppreference.net/c/language/arithmetic_types.html
- 7: https://learn.microsoft.com/en-us/windows/win32/winprog/windows-data-types
- 8: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/strtol-wcstol-strtol-l-wcstol-l?view=msvc-170
- 9: https://cppreference.com/cpp/string/stoi
Add focused coverage for 64-bit Select metrics.
Feed a synthetic Stats event to SelectHandler::DataFunction with all metrics above INT32_MAX, and assert the callback receives the exact long long values. This avoids requiring a multi-gigabyte object and prevents a regression from std::stoll to std::stol on Windows.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/select.cc` around lines 198 - 212, Add focused test coverage for
SelectHandler::DataFunction by supplying a synthetic Stats event whose
BytesScanned, BytesProcessed, and BytesReturned values all exceed INT32_MAX,
then assert the callback receives the exact long long values. Keep the test
independent of large objects and ensure it would detect any regression from
std::stoll to std::stol, particularly on Windows.
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
1 participant
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.
fix #208
Fixes two Windows (LLP64) issues reported via the VCPKG port: binary file corruption on download and 32-bit 'long' overflow for files larger than 2 GiB.
Binary-safe downloads:
Large file support (LLP64: 'long' is strictly 32-bit on Windows):
No behavioral change on LP64 platforms (Linux/macOS), where 'long' is already 64-bit.
Files changed: include/miniocpp/args.h, include/miniocpp/types.h, src/args.cc, src/baseclient.cc, src/client.cc, src/select.cc
Summary by CodeRabbit