<chrono>: Fix stack buffer overflow in chrono::parse() with modified widths#6270
Open
StephanTLavavej wants to merge 8 commits into
Open
<chrono>: Fix stack buffer overflow in chrono::parse() with modified widths#6270StephanTLavavej wants to merge 8 commits into
<chrono>: Fix stack buffer overflow in chrono::parse() with modified widths#6270StephanTLavavej wants to merge 8 commits into
Conversation
…nt()`. Testing against `cend(_Ac) - 1` is the fix. Add a comment explaining why. Moving `*_Ptr = _Ch;` inside the bounds test isn't necessary for the fix, but avoids potentially wacky behavior. If we continue to consume digits, we should exhibit the commented behavior of "drop trailing digits if already too large". Previously, we would keep updating the last digit, which makes no sense.
Add the same comment to the `_Ptr < _Pe` test. Similarly, move `*_Ptr = _Ch;` inside the bounds test, in accordance with the "drop trailing digits if already too large" comment. AFAICT, this has no behavioral impact since we're additionally limited by `_Digits_read < _Hi_digits` and `_Hi_digits` is at most 4.
We try to call test functions in definition order, and we usually test LWG resolutions last.
…d of `basic_stringstream`.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes a stack buffer overflow in chrono::parse() when parsing fields with modified widths, and extends the test suite to cover the corrected behavior.
Changes:
- Fix off-by-one buffer bounds checks in
chrono::_Time_parse_fields::_Get_int()and<xloctime>’s_Getint_v2()to preserve space for the null terminator. - Improve parsing tests by adding coverage for modified maximum character widths and enhancing diagnostics via
source_location. - Minor test refactors: rename
test_parse()runner, reorder LWG tests, and tighten stream types (istringstream/ostringstream).
Show a summary per file
| File | Description |
|---|---|
| tests/std/tests/P0355R7_calendars_and_time_zones_io/test.cpp | Adds diagnostics and new regression tests for modified-width parsing; minor test refactors. |
| stl/inc/xloctime | Adjusts digit-copy loop to avoid writing past the buffer when leaving room for a null terminator. |
| stl/inc/chrono | Fixes off-by-one bounds check to prevent stack buffer overwrite during integer field parsing. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 4
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.
Fixes #6269. (This bug was found by Copilot CLI with Opus 4.7, but this writeup, fix, and tests are all mine.)
chrono::_Time_parse_fields::_Get_int().cend(_Ac) - 1is the fix. Add a comment explaining why. Moving*_Ptr = _Ch;inside the bounds test isn't necessary for the fix, but avoids potentially wacky behavior. If we continue to consume digits, we should exhibit the commented behavior of "drop trailing digits if already too large". Previously, we would keep updating the last digit, which makes no sense.<xloctime>: Improve_Getint_v2()._Ptr < _Petest. Similarly, move*_Ptr = _Ch;inside the bounds test, in accordance with the "drop trailing digits if already too large" comment. AFAICT, this has no behavioral impact since we're additionally limited by_Digits_read < _Hi_digitsand_Hi_digitsis at most 4.test_parse()=>test_all_parse()test_lwg_3536(),test_lwg_3956(), no other changes.test_parse(),fail_parse()to takebasic_stringand usesource_location.29dwhen testingyear_month_day.basic_istringstream,basic_ostringstreaminstead ofbasic_stringstream.parse_modified_maximum_characters()