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

<xhash>: linking libs built with VS2017 into client code built with vs2019 can cause subtle memory corruption #2489

Closed
drink18 opened this issue Jan 19, 2022 · 4 comments · Fixed by #2774
Labels
bug Something isn't working fixed Something works now, yay!

Comments

@drink18
Copy link

drink18 commented Jan 19, 2022

Describe the bug
If you link a lib built with VS2017 into your client code built with vs2019, you may experience potential memory corruption.

  1. The direct cause/symptom is triggered by _Big_allocation_threshold in , where we force allocate more memory and align the block to 32bytes when allocating more than 4KB. The real address returned by underlying allocator is stored 8 bytes before the address returned to caller.
  2. When deallocating, _Deallocate(void* _Ptr, const size_t _Bytes) in will restore the above mentioned address from the address passed in by caller, if it sees the size to be deallocated is above _Big_allocation_threshold.
  3. This logic relies heavy on the correctness of memory size passed into _Deallocate.
  4. in vs2017, the std::_Hash implement its hash table with a std::vector, whereas this has been heavily refactored in VS2019, using _Hash_vec, which seems to be a specialized vector whose size() is always capacity(). Based on this strong assumption, it uses size() as the memory size parameter to call _Deallocate, when a memory deallocation happens.
  5. Now, if you mix libs (built with 2017) and calling code (built with 2019) together, when codes in libs manipulate unorderd_map, it potential breaks the assumption in the newer version of std::_Hash, as in case of std::vector, there is no such constraint size() == capacity(). When the unorderd_map (or any thing based on std::_Hash ) is destructed in calling code (built with vs2019), we could potentially pass misaligned pointer into underlying memory allocator, causing either a crash or a subtle memory corruption ( in my case a very hard to reproduce bug).
  6. The problem can be subtle to reproduce, as you have to keep adding more elements into unordered_map till you reach _Big_allocation_threshold, then remove some keys so that the assumed memory size returned by size() of _Vec in xhash is below _Big_allocation_threshold , then you have to destruct the unorderd_map in client code to trigger it.

I hope I have explained as clearly as possible the issue. I think this worth a proper fix. However I cannot think of one right off the top of my head.
In a real world, it's not always possible to control which toolchain a third-party library uses, and the potential harm of this issue can be huge.

Expected behavior
The newer std::_Hash needs to consider this edge case if possible.

STL version
Visual Studio Community 14.16.27023

This is also DevCom-1190124/VSO-1220461/AB#1220461

@StephanTLavavej StephanTLavavej added the bug Something isn't working label Jan 19, 2022
@StephanTLavavej
Copy link
Member

Thanks for reporting and investigating this issue. We'll need to look into this further - I am uncertain whether it's possible to fix this without wreaking further ABI havoc.

@frederick-vs-ja
Copy link
Contributor

Will #2774 fix this?

@CaseyCarter
Copy link
Member

Will #2774 fix this?

Yes it will - amazing catch! I'll get this bug linked up to the PR and the corresponding internal bug. Thanks!

@drink18
Copy link
Author

drink18 commented Jun 9, 2022

Great!
Will save millions of lives.

@StephanTLavavej StephanTLavavej added the fixed Something works now, yay! label Jun 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed Something works now, yay!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants