Skip to content

Fix heap-use-after-free for _HAS_EXCEPTIONS=0 - #5406

Merged
Stephan T. Lavavej (StephanTLavavej) merged 7 commits into
microsoft:mainfrom
wolframw:fix_5276
Apr 22, 2025
Merged

Fix heap-use-after-free for _HAS_EXCEPTIONS=0#5406
Stephan T. Lavavej (StephanTLavavej) merged 7 commits into
microsoft:mainfrom
wolframw:fix_5276

Conversation

@wolframw

Copy link
Copy Markdown
Contributor

Fixes #5276.

@wolframw
wolframw requested a review from a team as a code owner April 13, 2025 17:31
@github-project-automation github-project-automation Bot moved this to Initial Review in STL Code Reviews Apr 13, 2025
@wolframw

Copy link
Copy Markdown
Contributor Author

@microsoft-github-policy-service agree

@frederick-vs-ja

Copy link
Copy Markdown
Contributor

Can we delete (= delete;) the ctors from string when _HAS_EXCEPTIONS = 0? Stephan T. Lavavej (@StephanTLavavej) wolframw

@StephanTLavavej

Copy link
Copy Markdown
Member

We do have some precedent of outright =deleteing Standard APIs when compiler options make them literally unimplementable (e.g. dynamic_pointer_cast is deleted under /GR-).

In this case, because the option is ancient and the affected constructors (e.g. system_error) are old, I think that's a bit risky. Yes, we're dropping the string information on the floor, but we're preserving the type information in the string literal being stored. That seems like a "best effort" thing to do, and is strictly less problematic for users than the status quo.

Comment thread tests/std/tests/GH_005276_system_error_heap-use-after-free/test.cpp Outdated
Comment thread stl/inc/chrono Outdated
Comment thread stl/inc/chrono Outdated
Comment thread tests/std/test.lst Outdated
Comment thread tests/std/tests/GH_005276_system_error_heap-use-after-free/test.cpp Outdated
Comment thread tests/std/tests/GH_005276_system_error_heap-use-after-free/test.cpp Outdated
Comment thread tests/std/tests/GH_005276_system_error_heap-use-after-free/test.cpp Outdated
Comment thread tests/std/tests/GH_005276_system_error_heap-use-after-free/test.cpp Outdated
@StephanTLavavej

Copy link
Copy Markdown
Member

Thanks for fixing this surprisingly long-standing correctness bug! 😻 I pushed some test changes, and double-checked that the product code changes are complete.

Note for the future: we recommend against force-pushing after code review has begun, because GitHub makes it harder to see what's changed.

We merge PRs simultaneously to GitHub and our MSVC-internal repo in a semi-manual process, batched up to save time. Your PR will be part of the next batch, possibly this week but more likely next week depending on how busy I am. I'll post comments here as I prepare your PR for merging!

@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) moved this from Initial Review to Ready To Merge in STL Code Reviews Apr 16, 2025
@wolframw

Copy link
Copy Markdown
Contributor Author

Stephan T. Lavavej (@StephanTLavavej) Thanks for giving it the finishing touch!

@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) moved this from Ready To Merge to Merging in STL Code Reviews Apr 22, 2025
@StephanTLavavej

Copy link
Copy Markdown
Member

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

@StephanTLavavej
Stephan T. Lavavej (StephanTLavavej) merged commit bb031e2 into microsoft:main Apr 22, 2025
@github-project-automation github-project-automation Bot moved this from Merging to Done in STL Code Reviews Apr 22, 2025
@StephanTLavavej

Stephan T. Lavavej (StephanTLavavej) commented Apr 22, 2025

Copy link
Copy Markdown
Member

Thanks for fixing this bug and congratulations on your first microsoft/STL commit! 💚 😻 🎉

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

None yet

Development

Successfully merging this pull request may close these issues.

<system_error>: heap-use-after-free for _HAS_EXCEPTIONS=0

4 participants