sigsegv_handler: handle case when it is called on original stack #16276
Conversation
kbaladurin
force-pushed
the
signal_handler
branch
from
ea48052 to
bf86b9f
Compare
|
@dotnet-bot test Ubuntu x64 Checked Innerloop Build and Test |
|
@dotnet-bot test Alpine.3.6 x64 Debug Build |
|
@kbaladurin thank you for looking into the issue. I am sorry for a delayed response, I was on vacation last week. I would prefer making the necessary change in a simpler way. I would check for the alternate stack in the
|
|
@janvorli thank you for suggestion! I missed that sigaltstack isn't async-signal-safe. Also we have problems with First access to TLS variable: So we should fix this behavior too. Seems like we can specify TLS model for particular variable using In this case TLS was allocated before first access. |
|
|
|
@kbaladurin thank you for finding the problematic cases we already have! |
|
@janvorli thank you! Seems like void *
__pthread_getspecific (pthread_key_t key)
{
struct pthread_key_data *data;
/* Special case access to the first 2nd-level block. This is the
usual case. */
if (__glibc_likely (key < PTHREAD_KEY_2NDLEVEL_SIZE))
data = &THREAD_SELF->specific_1stblock[key];
else
{
/* Verify the key is sane. */
if (key >= PTHREAD_KEYS_MAX)
/* Not valid. */
return NULL;
unsigned int idx1st = key / PTHREAD_KEY_2NDLEVEL_SIZE;
unsigned int idx2nd = key % PTHREAD_KEY_2NDLEVEL_SIZE;
/* If the sequence number doesn't match or the key cannot be defined
for this thread since the second level array is not allocated
return NULL, too. */
struct pthread_key_data *level2 = THREAD_GETMEM_NC (THREAD_SELF,
specific, idx1st);
if (level2 == NULL)
/* Not allocated, therefore no data. */
return NULL;
/* There is data. */
data = &level2[idx2nd];
}
void *result = data->data;
if (result != NULL)
{
uintptr_t seq = data->seq;
if (__glibc_unlikely (seq != __pthread_keys[key].seq))
result = data->data = NULL;
}
return result;
}
sysdeps/x86_64/nptl/tls.h:
# define THREAD_GETMEM_NC(descr, member, idx) \
({ __typeof (descr->member[0]) __value; \
if (sizeof (__value) == 1) \
asm volatile ("movb %%fs:%P2(%q3),%b0" \
: "=q" (__value) \
: "0" (0), "i" (offsetof (struct pthread, member[0])), \
"r" (idx)); \
else if (sizeof (__value) == 4) \
asm volatile ("movl %%fs:%P1(,%q2,4),%0" \
: "=r" (__value) \
: "i" (offsetof (struct pthread, member[0])), "r" (idx));\
else \
{ \
if (sizeof (__value) != 8) \
/* There should not be any value with a size other than 1, \
4 or 8. */ \
abort (); \
\
asm volatile ("movq %%fs:%P1(,%q2,8),%q0" \
: "=r" (__value) \
: "i" (offsetof (struct pthread, member[0])), \
"r" (idx)); \
} \
__value; })
sysdeps/arm/nptl/tls.h:
#define THREAD_GETMEM_NC(descr, member, idx) \
descr->member[idx]
sysdeps/i386/nptl/tls.h:
# define THREAD_GETMEM_NC(descr, member, idx) \
({ __typeof (descr->member[0]) __value; \
if (sizeof (__value) == 1) \
asm volatile ("movb %%gs:%P2(%3),%b0" \
: "=q" (__value) \
: "0" (0), "i" (offsetof (struct pthread, member[0])), \
"r" (idx)); \
else if (sizeof (__value) == 4) \
asm volatile ("movl %%gs:%P1(,%2,4),%0" \
: "=r" (__value) \
: "i" (offsetof (struct pthread, member[0])), \
"r" (idx)); \
else \
{ \
if (sizeof (__value) != 8) \
/* There should not be any value with a size other than 1, \
4 or 8. */ \
abort (); \
\
asm volatile ("movl %%gs:%P1(,%2,8),%%eax\n\t" \
"movl %%gs:4+%P1(,%2,8),%%edx" \
: "=&A" (__value) \
: "i" (offsetof (struct pthread, member[0])), \
"r" (idx)); \
} \
__value; })So it's enough to fix |
|
@kbaladurin the thing that makes me nervous about using pthread_getspecific is that it is not guaranteed to be async signal safe. So even though it is that way today, it doesn't have to be the same way tomorrow. Although it seems that it was actually fixed to be async signal safe in glibc three years ago, so it sounds unlikely that it would be changed back to non-safe in the future. |
|
cc @mlabiuk |
kbaladurin
force-pushed
the
signal_handler
branch
from
bf86b9f to
25c499d
Compare
kbaladurin
changed the title
|
@janvorli thank you! It's better to replace pthread_set/getspecific by __thread in this case. I could make this changes in separate PR. |
src/pal/src/exception/signal.cpp
Outdated
|
|
||
| // Use should use initial-exec TLS model for this variable because access to it should be async-signal-safe as we | ||
| // want to use it in common_signal_handler | ||
| static __thread bool g_has_alternate_stack __attribute__ ((tls_model("initial-exec"))) = false; |
There was a problem hiding this comment.
I was thinking about this a bit and I've figured we actually don't need to a add a new variable. We can use the result of the GetCurrentPalThread() - if it returns null, the current thread is foreign and so it doesn't have our alternate stack. If it returns non-null, it has the alternate stack.
There was a problem hiding this comment.
Thank you! I've removed unnecessary variable.
kbaladurin
force-pushed
the
signal_handler
branch
from
25c499d to
dc18fee
Compare
src/pal/src/exception/signal.cpp
Outdated
| contextInitialization = false; | ||
| ExecuteHandlerOnOriginalStack(code, siginfo, context, &returnPoint); | ||
| _ASSERTE(FALSE); // The ExecuteHandlerOnOriginalStack should never return | ||
| if (GetCurrentPalThread()) |
There was a problem hiding this comment.
One last nit - the returnPoint structure is quite large due to the CONTEXT it contains. That unnecessarily adds to the stack of foreign threads, which might cause a stack overflow on distros with very small default stack, e.g. Alpine, in case when the SIGSEGV happens at a point when the foreign thread has only a low amount of stack space left. So it would be nice to make the check before the volatile bool contextInitialization = true and allocate the SignalHandlerWorkerReturnPoint using alloca only for the case when we need it. Btw, the address returned by alloca would need to be manually aligned to alignof(SignalHandlerWorkerReturnPoint) and allocate sizeof(SignalHandlerWorkerReturnPoint) + alignof(SignalHandlerWorkerReturnPoint) - 1 bytes.
There was a problem hiding this comment.
Done. Thanks for suggestion!
InternalGetCurrentThread in IsEnabled method. InternalGetCurrentThread tries to create pal thread if it doesn't exist for the current thread. It's unnecessary because in this case there are no hardware exception handlers for such thread. Also CatchHardwareExceptionHolder::IsEnable is called from signal handlers and during pal thread creation non-async-signal-safe function are called.
We should use initial-exec tls model to avoid memory allocations during first access to this variable because it may ocuur in signal handlers.
If sigsegv_handler is called on original stack (for example, if segmentation fault occurs in native application's thread that hasn't alternate signal stack) we should call common_signal_handler directly othersize sigsegv_handler's stackframe will be corrupted.
kbaladurin
force-pushed
the
signal_handler
branch
from
dc18fee to
29ff424
Compare
4 participants
If sigsegv_handler is called on original stack (for example, if segmentation fault occurs in native application's thread that hasn't alternate signal stack) we should call common_signal_handler directly othersize sigsegv_handler's stackframe will be corrupted.
Such kind of crash can be reproduced using following example:
libtest.c:In some cases it can lead to stack smashing errors that are described in https://github.com/dotnet/coreclr/issues/16208