Repository navigation
Conversation
The bare "%I" length modifier (without an explicit "32" or "64" suffix) is Microsoft's non-standard way to size an integer conversion to the platform's native pointer/size_t width: 4 bytes on 32-bit Windows, 8 bytes on 64-bit Windows. parse_flags() instead based the choice on SIZEOF_CURL_OFF_T, which is 8 on both, so on 32-bit Windows a bare "%In"/"%Id"/"%Iu" was always treated as 8-byte wide. For "%n" this made the MTYPE_INTPTR sink write an 8-byte int64_t through a caller-supplied pointer to a 4-byte destination, corrupting adjacent memory; for "%Id"/"%Iu" it fetched 8 bytes from the argument list where the caller passed a 4-byte value. Switch the bare "%I" case to SIZEOF_SIZE_T, matching the same pattern already used for "%z". Explicit "%I32"/"%I64" are unaffected. curl's own code does not use the bare "%I" modifier anywhere. Added test 1451 for verification (Windows-only). Reported-by: Joshua Rogers
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Test 1451 does not select libtests, so the new regression test will not execute.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
What changed in this PR
Corrects bare Windows %I handling to use native size_t width and adds regression coverage.
Changes:
- Selects width using
SIZEOF_SIZE_T. - Adds Windows tests for
%Inand%Iu. - Registers test 1451.
| File | Description |
|---|---|
lib/mprintf.c |
Corrects bare %I width selection. |
tests/libtest/lib1451.c |
Adds formatting regression checks. |
tests/libtest/Makefile.inc |
Registers the libtest source. |
tests/data/test1451 |
Defines the Windows test case. |
tests/data/Makefile.am |
Registers test 1451. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
The bare "%I" length modifier (without an explicit "32" or "64" suffix) is Microsoft's non-standard way to size an integer conversion to the platform's native pointer/size_t width: 4 bytes on 32-bit Windows, 8 bytes on 64-bit Windows. parse_flags() instead based the choice on SIZEOF_CURL_OFF_T, which is 8 on both, so on 32-bit Windows a bare "%In"/"%Id"/"%Iu" was always treated as 8-byte wide. For "%n" this made the MTYPE_INTPTR sink write an 8-byte int64_t through a caller-supplied pointer to a 4-byte destination, corrupting adjacent memory; for "%Id"/"%Iu" it fetched 8 bytes from the argument list where the caller passed a 4-byte value.
Switch the bare "%I" case to SIZEOF_SIZE_T, matching the same pattern already used for "%z". Explicit "%I32"/"%I64" are unaffected. curl's own code does not use the bare "%I" modifier anywhere.
Added test 1451 for verification (Windows-only).
Reported-by: Joshua Rogers