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

4 participants