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 stack overflow protection mechanism #553

Conversation

alexey-katranov
Copy link
Contributor

@alexey-katranov alexey-katranov commented Aug 30, 2021

The stack size is incorrectly supposed to be 4 MB on Windows. However, MSDN states that "The default stack reservation size used by the linker is 1 MB". It leads to two issues:

  1. The stack overflow protection mechanism does not work on Windows properly, i.e. stealing can be allowed even if the stack is fully consumed.
  2. The stack anchor calculation might overflow and block the stealing completely (that is reproduced in Some tests failed on MSVC 2019 (x64 build) #478 (comment)) due to main thread stack mapping on low address).

The fix changes the assumption to 1 MB on Windows. If compiled with modern Windows, the system API is used to query stack size instead of the assumption.

Comment on lines 94 to 96
ULONG_PTR hi, lo;
GetCurrentThreadStackLimits(&lo, &hi);
return std::size_t(hi - lo);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To support Windows 7, consider using NtCurrentTeb to query StackLimit from NT_TIB.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While it seems we can emulate GetCurrentThreadStackLimits via TIB, we should use DeallocationStack (not
StackLimit) for the real stack address. The implementation GetCurrentThreadStackLimits does the same (x64 implementation):

mov         r8,qword ptr gs:[30h]      // get TIB
mov         rax,qword ptr [r8+1478h]   // TIB + 1478h is DeallocationStack
mov         qword ptr [rcx],rax        // return LowLimit
mov         rax,qword ptr [r8+8]       // TIB + 8h is StackBase
mov         qword ptr [rdx],rax        // return HighLimit
ret

So, on x86_32 the DeallocationStack offset will be another and it seems the current approach is the lesser of two evils.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had no crashes on tests on Windows 7 (x86 and x64) after applying this patch.
The simple 1 MB limit works in all my Windows 7 environments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ULONG_PTR hi, lo;
GetCurrentThreadStackLimits(&lo, &hi);
return std::size_t(hi - lo);
ULONG_PTR hi, lo;
GetCurrentThreadStackLimits(&lo, &hi);
return static_cast<std::size_t>(hi - lo);

Copy link
Contributor

@anton-potapov anton-potapov Nov 16, 2021

Choose a reason for hiding this comment

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

I believe type cast is not needed here at whole, but if you wish...

@phprus
Copy link
Contributor

phprus commented Oct 30, 2021

Is there any progress?

@alexey-katranov
Copy link
Contributor Author

Thank you for the reminder, unfortunately, there is no any progress.

src/tbb/governor.cpp Outdated Show resolved Hide resolved
src/tbb/governor.cpp Outdated Show resolved Hide resolved
alexey-katranov and others added 3 commits November 19, 2021 13:14
Signed-off-by: Alexei Katranov <[email protected]>
Co-authored-by: Anton Potapov <[email protected]>
Signed-off-by: Alexei Katranov <[email protected]>
@alexey-katranov alexey-katranov force-pushed the dev/alexei-katranov/fix-stack-overflow-protection-mechanism branch from 0a6f3f8 to 384fc99 Compare November 19, 2021 10:29
Copy link
Contributor

@anton-potapov anton-potapov left a comment

Choose a reason for hiding this comment

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

LGTM

@anton-potapov anton-potapov merged commit 7631793 into master Nov 19, 2021
@anton-potapov anton-potapov deleted the dev/alexei-katranov/fix-stack-overflow-protection-mechanism branch November 19, 2021 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants