Skip to content

Conversation

wolframw
Copy link
Contributor

Fixes #5276.

@wolframw
Copy link
Contributor Author

@microsoft-github-policy-service agree

@StephanTLavavej StephanTLavavej added the bug Something isn't working label Apr 14, 2025
@StephanTLavavej StephanTLavavej self-assigned this Apr 14, 2025
@frederick-vs-ja
Copy link
Contributor

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

@StephanTLavavej
Copy link
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.

@StephanTLavavej
Copy link
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 StephanTLavavej removed their assignment Apr 16, 2025
@StephanTLavavej StephanTLavavej moved this from Initial Review to Ready To Merge in STL Code Reviews Apr 16, 2025
@wolframw
Copy link
Contributor Author

@StephanTLavavej Thanks for giving it the finishing touch!

@StephanTLavavej StephanTLavavej moved this from Ready To Merge to Merging in STL Code Reviews Apr 22, 2025
@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 bb031e2 into microsoft:main Apr 22, 2025
39 checks passed
@github-project-automation github-project-automation bot moved this from Merging to Done in STL Code Reviews Apr 22, 2025
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Apr 22, 2025

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

@wolframw wolframw deleted the fix_5276 branch April 22, 2025 23:15
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
Development

Successfully merging this pull request may close these issues.

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