Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Prevent memory allocation in signal handler #16384

Merged
merged 5 commits into from
Feb 20, 2018
Merged

Prevent memory allocation in signal handler #16384

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dfa0a5b
Prevent memory allocation in signal handler
mlabiuk Feb 13, 2018
ee26488
Revert "Prevent memory allocation in signal handler"
mlabiuk Feb 19, 2018
f8dafbb
Do not allocate exception for signal from non managed code
mlabiuk Feb 19, 2018
e7bf54e
Move exception allocation to PAL_SEHException
mlabiuk Feb 20, 2018
5966e0e
Remove exception records allocation from pal.h
mlabiuk Feb 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/pal/inc/pal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3762,6 +3762,8 @@ PAL_BindResources(IN LPCSTR lpDomain);

#define EXCEPTION_IS_SIGNAL 0x100

#define EXCEPTION_ON_STACK 0x400
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer adding a bool member to the PAL_SEHException (and a corresponding argument to the constructor) instead to indicate whether exception records are on stack or allocated by the allocator. It feels a bit better due to the fact that it is related to both of the exception and context records. Also, as for the name, I'd prefer something like "RecordsOnStack" since it refers to the records instead of the exception itself.


#define EXCEPTION_MAXIMUM_PARAMETERS 15

// Index in the ExceptionInformation array where we will keep the reference
Expand Down Expand Up @@ -5683,7 +5685,8 @@ struct PAL_SEHException

void FreeRecords()
{
if (ExceptionPointers.ExceptionRecord != NULL)
if (ExceptionPointers.ExceptionRecord != NULL &&
! (ExceptionPointers.ExceptionRecord->ExceptionFlags | EXCEPTION_ON_STACK) )
Copy link
Member

Choose a reason for hiding this comment

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

This second part of the condition is not correct, it should be & instead of |.

{
PAL_FreeExceptionRecords(ExceptionPointers.ExceptionRecord, ExceptionPointers.ContextRecord);
ExceptionPointers.ExceptionRecord = NULL;
Expand Down
23 changes: 23 additions & 0 deletions src/pal/src/exception/seh.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,23 @@ void ThrowExceptionHelper(PAL_SEHException* ex)
throw std::move(*ex);
}

static PAL_SEHException copyPAL_SEHException(PAL_SEHException* src)
Copy link
Member

Choose a reason for hiding this comment

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

Rather than creating a new copy, I would prefer updating the existing exception instance. Either directly or by adding a Set method to the PAL_SEHException with the same arguments as the constructor.
Also, the prevalent coding convention in PAL is to use pascal casing for function names without underscores. The PAL_ prefix is an exception, indicating functions and data structures that are exported by PAL, but don't exist on Windows.
How about naming it e.g. "MoveExceptionRecordsToHeap" or "EnsureExceptionRecordsOnHeap"?

{
CONTEXT* contextRecord = src->GetContextRecord();
EXCEPTION_RECORD* exceptionRecord = src->GetExceptionRecord();

CONTEXT* contextRecordCopy;
EXCEPTION_RECORD* exceptionRecordCopy;
AllocateExceptionRecords(&exceptionRecordCopy, &contextRecordCopy);

*exceptionRecordCopy = *exceptionRecord;
exceptionRecordCopy->ExceptionFlags &= ~EXCEPTION_ON_STACK;
*contextRecordCopy = *contextRecord;
return PAL_SEHException(exceptionRecordCopy, contextRecordCopy);
}



/*++
Function:
SEHProcessException
Expand Down Expand Up @@ -249,6 +266,9 @@ SEHProcessException(PAL_SEHException* exception)
PROCAbort();
}
}

if(exceptionRecord->ExceptionFlags | EXCEPTION_ON_STACK)
*exception = copyPAL_SEHException(exception);
Copy link
Member

Choose a reason for hiding this comment

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

Could you please use the braces even for single line body to match the surrounding code style?


if (g_hardwareExceptionHandler(exception))
{
Expand All @@ -262,6 +282,9 @@ SEHProcessException(PAL_SEHException* exception)

if (CatchHardwareExceptionHolder::IsEnabled())
{
if(exceptionRecord->ExceptionFlags | EXCEPTION_ON_STACK)
Copy link
Member

Choose a reason for hiding this comment

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

To simplify the callers, it would be nice to move the check into the actual function that updates the exception. For exception that already has records on heap, the function would become a no-op.

*exception = copyPAL_SEHException(exception);

PAL_ThrowExceptionFromContext(exception->GetContextRecord(), exception);
}
}
Expand Down
30 changes: 15 additions & 15 deletions src/pal/src/exception/signal.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -826,32 +826,32 @@ static bool common_signal_handler(int code, siginfo_t *siginfo, void *sigcontext
{
sigset_t signal_set;
CONTEXT signalContextRecord;
CONTEXT *contextRecord;
EXCEPTION_RECORD *exceptionRecord;
CONTEXT contextRecord;
EXCEPTION_RECORD exceptionRecord;
native_context_t *ucontext;

ucontext = (native_context_t *)sigcontext;
g_common_signal_handler_context_locvar_offset = (int)((char*)&signalContextRecord - (char*)__builtin_frame_address(0));

AllocateExceptionRecords(&exceptionRecord, &contextRecord);
//AllocateExceptionRecords(&exceptionRecord, &contextRecord);
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the commented out code.


exceptionRecord->ExceptionCode = CONTEXTGetExceptionCodeForSignal(siginfo, ucontext);
exceptionRecord->ExceptionFlags = EXCEPTION_IS_SIGNAL;
exceptionRecord->ExceptionRecord = NULL;
exceptionRecord->ExceptionAddress = GetNativeContextPC(ucontext);
exceptionRecord->NumberParameters = numParams;
exceptionRecord.ExceptionCode = CONTEXTGetExceptionCodeForSignal(siginfo, ucontext);
exceptionRecord.ExceptionFlags = EXCEPTION_IS_SIGNAL | EXCEPTION_ON_STACK;
exceptionRecord.ExceptionRecord = NULL;
exceptionRecord.ExceptionAddress = GetNativeContextPC(ucontext);
exceptionRecord.NumberParameters = numParams;

va_list params;
va_start(params, numParams);

for (int i = 0; i < numParams; i++)
{
exceptionRecord->ExceptionInformation[i] = va_arg(params, size_t);
exceptionRecord.ExceptionInformation[i] = va_arg(params, size_t);
}

// Pre-populate context with data from current frame, because ucontext doesn't have some data (e.g. SS register)
// which is required for restoring context
RtlCaptureContext(contextRecord);
RtlCaptureContext(&contextRecord);

ULONG contextFlags = CONTEXT_CONTROL | CONTEXT_INTEGER | CONTEXT_FLOATING_POINT;

Expand All @@ -862,7 +862,7 @@ static bool common_signal_handler(int code, siginfo_t *siginfo, void *sigcontext
// Fill context record with required information. from pal.h:
// On non-Win32 platforms, the CONTEXT pointer in the
// PEXCEPTION_POINTERS will contain at least the CONTEXT_CONTROL registers.
CONTEXTFromNativeContext(ucontext, contextRecord, contextFlags);
CONTEXTFromNativeContext(ucontext, &contextRecord, contextFlags);

/* Unmask signal so we can receive it again */
sigemptyset(&signal_set);
Expand All @@ -873,17 +873,17 @@ static bool common_signal_handler(int code, siginfo_t *siginfo, void *sigcontext
ASSERT("pthread_sigmask failed; error number is %d\n", sigmaskRet);
}

contextRecord->ContextFlags |= CONTEXT_EXCEPTION_ACTIVE;
contextRecord.ContextFlags |= CONTEXT_EXCEPTION_ACTIVE;

memcpy_s(&signalContextRecord, sizeof(CONTEXT), contextRecord, sizeof(CONTEXT));
memcpy_s(&signalContextRecord, sizeof(CONTEXT), &contextRecord, sizeof(CONTEXT));

// The exception object takes ownership of the exceptionRecord and contextRecord
PAL_SEHException exception(exceptionRecord, contextRecord);
PAL_SEHException exception(&exceptionRecord, &contextRecord);

if (SEHProcessException(&exception))
{
// Exception handling may have modified the context, so update it.
CONTEXTToNativeContext(contextRecord, ucontext);
CONTEXTToNativeContext(&contextRecord, ucontext);
return true;
}

Expand Down