Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly null-terminate output buffer in basic_istream::get[line] #5073

Merged
merged 8 commits into from
Nov 14, 2024

Conversation

muellerj2
Copy link
Contributor

Fixes #5070.

The libcxx tests istream.unformatted/get_pointer_size.pass.cpp and get_pointer_size_chart.pass.cpp pass now.

Since this issue is mostly about correct null-termination in the presence of exceptions, I added the tests to GH_001858_iostream_exception. They should cover the relevant cases:

  • output buffer of size zero with and without exception on failbit
  • sentry failure with and without exception on failbit
  • exception during streambuf character extraction with and without badbit exception propagation
  • encountering EOF with and without exception on eofbit

@muellerj2 muellerj2 requested a review from a team as a code owner November 10, 2024 20:29
@muellerj2
Copy link
Contributor Author

@microsoft-github-policy-service agree

stl/inc/istream Outdated Show resolved Hide resolved
stl/inc/istream Outdated Show resolved Hide resolved
tests/std/tests/GH_001858_iostream_exception/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/GH_001858_iostream_exception/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/GH_001858_iostream_exception/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/GH_001858_iostream_exception/test.cpp Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

Thanks, this is fantastic - and incredible for a first PR! 😻 I've pushed several minor nitpicks, a product code simplification, and a test code improvement. I appreciate the extremely detailed test coverage.

We merge PRs simultaneously to our GitHub and MSVC-internal repos, mirrored in a semi-manual process and batched up to save time. Your PR will be part of the next batch, very likely this week.

@StephanTLavavej StephanTLavavej removed their assignment Nov 11, 2024
@StephanTLavavej StephanTLavavej self-assigned this Nov 14, 2024
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit ca1af94 into microsoft:main Nov 14, 2024
39 checks passed
@StephanTLavavej
Copy link
Member

Thanks again, and congratulations on your first microsoft/STL commit! 🚀 😸 🥳

This change is expected to ship in VS 2022 17.13 Preview 3.

@muellerj2 muellerj2 deleted the istream-get-nulltermination branch November 23, 2024 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
2 participants