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

Fix put_time() crash on invalid struct tm data #4883

Merged

Conversation

TheStormN
Copy link
Contributor

@TheStormN TheStormN commented Aug 8, 2024

Fixes #4882
Fixes #924

My next PR about fixing std::put_time crashes.

If all goes well I will try to also fix the performance issues mentioned in #3575 and #1900 before VS 17.12 cut.

UPDATE: The result now outputs single ? for every invalid tm struct data based on specifier. For example if tm struct contains invalid year and the format string is "%Y-%m-%d-%H-%M", the output will be like ?-08-10-00-45.

@TheStormN TheStormN requested a review from a team as a code owner August 8, 2024 21:47
@StephanTLavavej StephanTLavavej added the bug Something isn't working label Aug 8, 2024
tests/std/tests/Dev11_0836436_get_time/test.cpp Outdated Show resolved Hide resolved
stl/inc/xloctime Show resolved Hide resolved
stl/inc/xloctime Outdated Show resolved Hide resolved
stl/inc/xloctime Outdated Show resolved Hide resolved
tests/std/tests/Dev11_0836436_get_time/test.cpp Outdated Show resolved Hide resolved
stl/inc/xloctime Outdated Show resolved Hide resolved
@TheStormN

This comment was marked as resolved.

@StephanTLavavej

This comment was marked as resolved.

@TheStormN TheStormN force-pushed the issue-4882-put_time-invalid-tm-crash branch from 44acdd6 to 37d7f3e Compare August 9, 2024 21:39
@TheStormN

This comment was marked as outdated.

@CaseyCarter

This comment was marked as outdated.

Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is looking really good! I'm about to fall asleep so I want to take another look at this, but here are the comments I noticed.

tests/std/tests/Dev11_0836436_get_time/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/Dev11_0836436_get_time/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/Dev11_0836436_get_time/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/Dev11_0836436_get_time/test.cpp Outdated Show resolved Hide resolved
@TheStormN TheStormN force-pushed the issue-4882-put_time-invalid-tm-crash branch from eae10b0 to bab2450 Compare August 10, 2024 14:56
@TheStormN

This comment was marked as resolved.

@TheStormN

This comment was marked as resolved.

@TheStormN TheStormN force-pushed the issue-4882-put_time-invalid-tm-crash branch from 005e5b8 to 3317d68 Compare August 10, 2024 20:28
@TheStormN

This comment was marked as resolved.

@fsb4000

This comment was marked as resolved.

@fsb4000

This comment was marked as resolved.

@TheStormN

This comment was marked as resolved.

@StephanTLavavej

This comment was marked as resolved.

@TheStormN

This comment was marked as resolved.

@StephanTLavavej

This comment was marked as resolved.

@TheStormN

This comment was marked as resolved.

@StephanTLavavej

This comment was marked as resolved.

@TheStormN

This comment was marked as resolved.

stl/inc/xloctime Outdated Show resolved Hide resolved
stl/inc/xloctime Outdated Show resolved Hide resolved
stl/inc/xloctime Outdated Show resolved Hide resolved
stl/inc/xloctime Outdated Show resolved Hide resolved
tests/std/tests/Dev11_0836436_get_time/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/Dev11_0836436_get_time/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/Dev11_0836436_get_time/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/Dev11_0836436_get_time/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/Dev11_0836436_get_time/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/Dev11_0836436_get_time/test.cpp Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej removed their assignment Aug 21, 2024
@TheStormN
Copy link
Contributor Author

TheStormN commented Aug 21, 2024

Man, if we could just push a bit the UCRT team to replicate at least the validation behavior of other C libraries regarding strftime, we would save so much pain. :)

@StephanTLavavej StephanTLavavej self-assigned this Aug 25, 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 5877281 into microsoft:main Aug 25, 2024
39 checks passed
@StephanTLavavej
Copy link
Member

Thanks for fixing this long-standing bug and making this code so much more robust! 🎉 😸 🚀

@TheStormN TheStormN deleted the issue-4882-put_time-invalid-tm-crash branch August 25, 2024 17:51
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
5 participants