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

Fix debugger launch race hitting breakpoints in startup code. #8951

Merged
merged 1 commit into from
Jan 21, 2017
Merged

Fix debugger launch race hitting breakpoints in startup code. #8951

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 10 additions & 3 deletions src/debug/ee/debugger.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1898,7 +1898,7 @@ void Debugger::SendCreateProcess(DebuggerLockHolder * pDbgLockHolder)

#if defined(FEATURE_CORECLR) && !defined(FEATURE_PAL)

HANDLE g_hContinueStartupEvent = NULL;
HANDLE g_hContinueStartupEvent = INVALID_HANDLE_VALUE;

CLR_ENGINE_METRICS g_CLREngineMetrics = {
sizeof(CLR_ENGINE_METRICS),
Expand Down Expand Up @@ -1945,7 +1945,7 @@ void NotifyDebuggerOfTelestoStartup()
// enumeration of this process will get back a valid continue event
// the instant we signal the startup notification event.

CONSISTENCY_CHECK(NULL == g_hContinueStartupEvent);
CONSISTENCY_CHECK(INVALID_HANDLE_VALUE == g_hContinueStartupEvent);
g_hContinueStartupEvent = WszCreateEvent(NULL, TRUE, FALSE, NULL);
CONSISTENCY_CHECK(INVALID_HANDLE_VALUE != g_hContinueStartupEvent); // we reserve this value for error conditions in EnumerateCLRs

Expand Down Expand Up @@ -2173,7 +2173,14 @@ HRESULT Debugger::Startup(void)
// Signal the debugger (via dbgshim) and wait until it is ready for us to
// continue. This needs to be outside the lock and after the transport is
// initialized.
PAL_NotifyRuntimeStarted();
if (PAL_NotifyRuntimeStarted())
{
// The runtime was successfully launched and attached so mark it now
// so no notifications are missed especially the initial module load
// which would cause debuggers problems with reliable setting breakpoints
// in startup code or Main.
MarkDebuggerAttachedInternal();
}
#endif // FEATURE_PAL

// We don't bother changing this process's permission.
Expand Down
63 changes: 52 additions & 11 deletions src/dlls/dbgshim/dbgshim.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,25 +387,66 @@ class RuntimeStartupHelper
return hr;
}

bool AreAllHandlesValid(HANDLE *handleArray, DWORD arrayLength)
{
for (int i = 0; i < (int)arrayLength; i++)
{
HANDLE h = handleArray[i];
if (h == INVALID_HANDLE_VALUE)
Copy link

Choose a reason for hiding this comment

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

In the case of older CoreCLR's, this will still be NULL, so should we check both?

{
return false;
}
}
return true;
}

HRESULT InternalEnumerateCLRs(HANDLE** ppHandleArray, _In_reads_(*pdwArrayLength) LPWSTR** ppStringArray, DWORD* pdwArrayLength)
{
int numTries = 0;
HRESULT hr;

while (numTries < 25)
{
hr = EnumerateCLRs(m_processId, ppHandleArray, ppStringArray, pdwArrayLength);

// EnumerateCLRs uses the OS API CreateToolhelp32Snapshot which can return ERROR_BAD_LENGTH or
// ERROR_PARTIAL_COPY. If we get either of those, we try wait 1/10th of a second try again (that
// is the recommendation of the OS API owners)
hr = EnumerateCLRs(m_processId, ppHandleArray, ppStringArray, pdwArrayLength);
// is the recommendation of the OS API owners).
if ((hr != HRESULT_FROM_WIN32(ERROR_PARTIAL_COPY)) && (hr != HRESULT_FROM_WIN32(ERROR_BAD_LENGTH)))
{
break;
// Just return any other error or if no handles were found (which means the coreclr module wasn't found yet).
if (FAILED(hr) || *ppHandleArray == NULL || *pdwArrayLength <= 0)
{
return hr;
}
// If EnumerateCLRs succeeded but any of the handles are INVALID_HANDLE_VALUE, then sleep and retry
// also. This fixes a race condition where dbgshim catches the coreclr module just being loaded but
// before g_hContinueStartupEvent has been initialized.
if (AreAllHandlesValid(*ppHandleArray, *pdwArrayLength))
{
return hr;
}
Copy link

Choose a reason for hiding this comment

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

If we don't find anything, would it make more sense to wait for the startup event to get set? (ex:

        CloseCLREnumeration(*ppHandleArrayOut, *ppStringArrayOut, *pdwArrayLengthOut);
        *ppHandleArrayOut = NULL;
        *ppStringArrayOut = NULL;
        *pdwArrayLengthOut = 0;

        return S_OK;

If we do decide to retry, we need to free the memory before doing so or we will leak.

// Clean up memory allocated in EnumerateCLRs since this path it succeeded
CloseCLREnumeration(*ppHandleArray, *ppStringArray, *pdwArrayLength);

*ppHandleArray = NULL;
*ppStringArray = NULL;
*pdwArrayLength = 0;
}

// Sleep and retry enumerating the runtimes
Sleep(100);
numTries++;

if (m_canceled)
{
break;
}
}

// Indicate a timeout
hr = HRESULT_FROM_WIN32(ERROR_TIMEOUT);

return hr;
}

Expand Down Expand Up @@ -517,9 +558,8 @@ class RuntimeStartupHelper
bool coreclrExists = false;

HRESULT hr = InvokeStartupCallback(&coreclrExists);
// Because the target process is suspended on create, the toolhelp apis fail with the below errors even
// with the retry logic in InternalEnumerateCLRs.
if (SUCCEEDED(hr) || (hr == HRESULT_FROM_WIN32(ERROR_PARTIAL_COPY)) || (hr == HRESULT_FROM_WIN32(ERROR_BAD_LENGTH)))
// The retry logic in InternalEnumerateCLRs failed if ERROR_TIMEOUT was returned.
if (SUCCEEDED(hr) || (hr == HRESULT_FROM_WIN32(ERROR_TIMEOUT)))
{
if (!coreclrExists && !m_canceled)
{
Expand All @@ -531,8 +571,11 @@ class RuntimeStartupHelper
hr = InvokeStartupCallback(&coreclrExists);
if (SUCCEEDED(hr))
{
// We should always find a coreclr module
_ASSERTE(coreclrExists);
// We should always find a coreclr module so fail if we don't
if (!coreclrExists)
{
hr = E_FAIL;
}
}
}
}
Expand Down Expand Up @@ -1120,8 +1163,6 @@ EnumerateCLRs(

HANDLE hContinueStartupEvent = INVALID_HANDLE_VALUE;
HRESULT hr = GetContinueStartupEvent(debuggeePID, pStringArray[idx], &hContinueStartupEvent);
_ASSERTE(SUCCEEDED(hr) == (hContinueStartupEvent != INVALID_HANDLE_VALUE));

pEventArray[idx] = hContinueStartupEvent;
#else
pEventArray[idx] = NULL;
Expand Down Expand Up @@ -1729,7 +1770,7 @@ GetContinueStartupEvent(
ThrowHR(E_FAIL);
}

if (NULL != continueEvent)
if (NULL != continueEvent && INVALID_HANDLE_VALUE != continueEvent)
{
if (!DuplicateHandle(hProcess, continueEvent, GetCurrentProcess(), &continueEvent,
EVENT_MODIFY_STATE, FALSE, 0))
Expand Down
16 changes: 7 additions & 9 deletions src/pal/src/thread/process.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1879,7 +1879,7 @@ PAL_UnregisterForRuntimeStartup(
None

Return value:
TRUE - succeeded, FALSE - failed
TRUE - successfully launched by debugger, FALSE - not launched or some failure in the handshake
--*/
BOOL
PALAPI
Expand All @@ -1889,7 +1889,7 @@ PAL_NotifyRuntimeStarted()
char continueSemName[CLR_SEM_MAX_NAMELEN];
sem_t *startupSem = SEM_FAILED;
sem_t *continueSem = SEM_FAILED;
BOOL result = TRUE;
BOOL launched = FALSE;

UINT64 processIdDisambiguationKey = 0;
GetProcessIdDisambiguationKey(gPID, &processIdDisambiguationKey);
Expand All @@ -1899,9 +1899,7 @@ PAL_NotifyRuntimeStarted()

TRACE("PAL_NotifyRuntimeStarted opening continue '%s' startup '%s'\n", continueSemName, startupSemName);


// Open the debugger startup semaphore. If it doesn't exists, then we do nothing and
// the function is successful.
// Open the debugger startup semaphore. If it doesn't exists, then we do nothing and return
startupSem = sem_open(startupSemName, 0);
if (startupSem == SEM_FAILED)
{
Expand All @@ -1913,26 +1911,26 @@ PAL_NotifyRuntimeStarted()
if (continueSem == SEM_FAILED)
{
ASSERT("sem_open(%s) failed: %d (%s)\n", continueSemName, errno, strerror(errno));
result = FALSE;
goto exit;
}

// Wake up the debugger waiting for startup
if (sem_post(startupSem) != 0)
{
ASSERT("sem_post(startupSem) failed: errno is %d (%s)\n", errno, strerror(errno));
result = FALSE;
goto exit;
}

// Now wait until the debugger's runtime startup notification is finished
if (sem_wait(continueSem) != 0)
{
ASSERT("sem_wait(continueSem) failed: errno is %d (%s)\n", errno, strerror(errno));
result = FALSE;
goto exit;
}

// Returns that the runtime was successfully launched for debugging
launched = TRUE;

exit:
if (startupSem != SEM_FAILED)
{
Expand All @@ -1942,7 +1940,7 @@ PAL_NotifyRuntimeStarted()
{
sem_close(continueSem);
}
return result;
return launched;
}

/*++
Expand Down