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 undefined behavior found with g++-12 ubsan #73424

Merged
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
3 changes: 2 additions & 1 deletion src/coreclr/inc/utilcode.h
Original file line number Diff line number Diff line change
Expand Up @@ -1389,7 +1389,8 @@ T *CUnorderedArrayWithAllocator<T,iGrowInc,ALLOCATOR>::Grow() // exception if c

// try to allocate memory for reallocation.
pTemp = ALLOCATOR::AllocThrowing(this, m_iSize+iGrowInc);
memcpy (pTemp, m_pTable, m_iSize*sizeof(T));
if (m_iSize > 0)
memcpy (pTemp, m_pTable, m_iSize*sizeof(T));
ALLOCATOR::Free(this, m_pTable);
m_pTable = pTemp;
m_iSize += iGrowInc;
Expand Down
5 changes: 3 additions & 2 deletions src/coreclr/jit/compiler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1774,6 +1774,9 @@ void Compiler::compInit(ArenaAllocator* pAlloc,

#ifdef DEBUG
bRangeAllowStress = false;

// set this early so we can use it without relying on random memory values
verbose = compIsForInlining() ? impInlineInfo->InlinerCompiler->verbose : false;
#endif

#if defined(DEBUG) || defined(LATE_DISASM) || DUMP_FLOWGRAPHS
Expand Down Expand Up @@ -5561,8 +5564,6 @@ int Compiler::compCompile(CORINFO_MODULE_HANDLE classPtr,
#ifdef DEBUG
Compiler* me = this;
forceFrameJIT = (void*)&me; // let us see the this pointer in fastchecked build
// set this early so we can use it without relying on random memory values
verbose = compIsForInlining() ? impInlineInfo->InlinerCompiler->verbose : false;
#endif

#if FUNC_INFO_LOGGING
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/jit/gentree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9364,7 +9364,7 @@ GenTreeUseEdgeIterator::GenTreeUseEdgeIterator(GenTree* node)
return;

case GT_CALL:
m_statePtr = &*m_node->AsCall()->gtArgs.Args().begin();
m_statePtr = m_node->AsCall()->gtArgs.Args().begin().GetArg();
m_advance = &GenTreeUseEdgeIterator::AdvanceCall<CALL_ARGS>;
AdvanceCall<CALL_ARGS>();
return;
Expand Down Expand Up @@ -9649,7 +9649,7 @@ void GenTreeUseEdgeIterator::AdvanceCall()
return;
}
}
m_statePtr = &*call->gtArgs.LateArgs().begin();
m_statePtr = call->gtArgs.LateArgs().begin().GetArg();
m_advance = &GenTreeUseEdgeIterator::AdvanceCall<CALL_LATE_ARGS>;
FALLTHROUGH;

Expand Down
70 changes: 48 additions & 22 deletions src/coreclr/jit/lsra.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6681,15 +6681,19 @@ void LinearScan::resolveRegisters()

// handle incoming arguments and special temps
RefPositionIterator refPosIterator = refPositions.begin();
RefPosition* currentRefPosition = &refPosIterator;
RefPosition* currentRefPosition = refPosIterator != refPositions.end() ? &refPosIterator : nullptr;

if (enregisterLocalVars)
{
VarToRegMap entryVarToRegMap = inVarToRegMaps[compiler->fgFirstBB->bbNum];
for (; refPosIterator != refPositions.end() &&
(currentRefPosition->refType == RefTypeParamDef || currentRefPosition->refType == RefTypeZeroInit);
++refPosIterator, currentRefPosition = &refPosIterator)
for (; refPosIterator != refPositions.end(); ++refPosIterator)
{
currentRefPosition = &refPosIterator;
if (currentRefPosition->refType != RefTypeParamDef && currentRefPosition->refType != RefTypeZeroInit)
{
break;
}

Interval* interval = currentRefPosition->getInterval();
assert(interval != nullptr && interval->isLocalVar);
resolveLocalRef(nullptr, nullptr, currentRefPosition);
Expand Down Expand Up @@ -6731,9 +6735,14 @@ void LinearScan::resolveRegisters()
}

// Handle the DummyDefs, updating the incoming var location.
for (; refPosIterator != refPositions.end() && currentRefPosition->refType == RefTypeDummyDef;
++refPosIterator, currentRefPosition = &refPosIterator)
for (; refPosIterator != refPositions.end(); ++refPosIterator)
{
currentRefPosition = &refPosIterator;
if (currentRefPosition->refType != RefTypeDummyDef)
{
break;
}

assert(currentRefPosition->isIntervalRef());
// Don't mark dummy defs as reload
currentRefPosition->reload = false;
Expand All @@ -6756,13 +6765,17 @@ void LinearScan::resolveRegisters()
assert(refPosIterator != refPositions.end());
assert(currentRefPosition->refType == RefTypeBB);
++refPosIterator;
currentRefPosition = &refPosIterator;
currentRefPosition = refPosIterator != refPositions.end() ? &refPosIterator : nullptr;
AaronRobinsonMSFT marked this conversation as resolved.
Show resolved Hide resolved

// Handle the RefPositions for the block
AaronRobinsonMSFT marked this conversation as resolved.
Show resolved Hide resolved
for (; refPosIterator != refPositions.end() && currentRefPosition->refType != RefTypeBB &&
currentRefPosition->refType != RefTypeDummyDef;
++refPosIterator, currentRefPosition = &refPosIterator)
for (; refPosIterator != refPositions.end(); ++refPosIterator)
{
currentRefPosition = &refPosIterator;
if (currentRefPosition->refType == RefTypeBB || currentRefPosition->refType == RefTypeDummyDef)
{
break;
}

currentLocation = currentRefPosition->nodeLocation;

// Ensure that the spill & copy info is valid.
Expand Down Expand Up @@ -9488,9 +9501,14 @@ void LinearScan::TupleStyleDump(LsraTupleDumpMode mode)
if (mode != LSRA_DUMP_PRE)
{
printf("Incoming Parameters: ");
for (; refPosIterator != refPositions.end() && currentRefPosition->refType != RefTypeBB;
++refPosIterator, currentRefPosition = &refPosIterator)
for (; refPosIterator != refPositions.end(); ++refPosIterator)
{
currentRefPosition = &refPosIterator;
if (currentRefPosition->refType == RefTypeBB)
{
break;
}

Interval* interval = currentRefPosition->getInterval();
assert(interval != nullptr && interval->isLocalVar);
printf(" V%02d", interval->varNum);
Expand Down Expand Up @@ -9530,11 +9548,15 @@ void LinearScan::TupleStyleDump(LsraTupleDumpMode mode)
{
bool printedBlockHeader = false;
// We should find the boundary RefPositions in the order of exposed uses, dummy defs, and the blocks
for (; refPosIterator != refPositions.end() &&
(currentRefPosition->refType == RefTypeExpUse || currentRefPosition->refType == RefTypeDummyDef ||
(currentRefPosition->refType == RefTypeBB && !printedBlockHeader));
++refPosIterator, currentRefPosition = &refPosIterator)
for (; refPosIterator != refPositions.end(); ++refPosIterator)
{
currentRefPosition = &refPosIterator;
if (currentRefPosition->refType != RefTypeExpUse && currentRefPosition->refType != RefTypeDummyDef &&
!(currentRefPosition->refType == RefTypeBB && !printedBlockHeader))
AaronRobinsonMSFT marked this conversation as resolved.
Show resolved Hide resolved
{
break;
}

Interval* interval = nullptr;
if (currentRefPosition->isIntervalRef())
{
Expand Down Expand Up @@ -9613,13 +9635,17 @@ void LinearScan::TupleStyleDump(LsraTupleDumpMode mode)
// and combining the fixed regs with their associated def or use
bool killPrinted = false;
RefPosition* lastFixedRegRefPos = nullptr;
for (; refPosIterator != refPositions.end() &&
(currentRefPosition->refType == RefTypeUse || currentRefPosition->refType == RefTypeFixedReg ||
currentRefPosition->refType == RefTypeKill || currentRefPosition->refType == RefTypeDef) &&
(currentRefPosition->nodeLocation == tree->gtSeqNum ||
currentRefPosition->nodeLocation == tree->gtSeqNum + 1);
++refPosIterator, currentRefPosition = &refPosIterator)
for (; refPosIterator != refPositions.end(); ++refPosIterator)
{
currentRefPosition = &refPosIterator;
if (!(currentRefPosition->refType == RefTypeUse || currentRefPosition->refType == RefTypeFixedReg ||
currentRefPosition->refType == RefTypeKill || currentRefPosition->refType == RefTypeDef) ||
!(currentRefPosition->nodeLocation == tree->gtSeqNum ||
currentRefPosition->nodeLocation == tree->gtSeqNum + 1))
AaronRobinsonMSFT marked this conversation as resolved.
Show resolved Hide resolved
{
break;
}

Interval* interval = nullptr;
if (currentRefPosition->isIntervalRef())
{
Expand Down
22 changes: 12 additions & 10 deletions src/coreclr/vm/crst.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -327,9 +327,7 @@ void CrstBase::Enter(INDEBUG(NoLevelCheckFlag noLevelCheckFlag/* = CRST_LEVEL_CH

if (fToggle)
{

pThread->DisablePreemptiveGC();

}
}

Expand Down Expand Up @@ -419,14 +417,17 @@ void CrstBase::PreEnter()
}

// If a thread suspends another thread, it cannot acquire locks.
if ((pThread != NULL) &&
(pThread->Debug_GetUnsafeSuspendeeCount() != 0))
if ((pThread != NULL)
&& (pThread->Debug_GetUnsafeSuspendeeCount() != 0))
{
CONSISTENCY_CHECK_MSGF(false, ("Suspender thread taking non-suspender lock:'%s'", m_tag));
}

if (ThreadStore::s_pThreadStore->IsCrstForThreadStore(this))
if ((ThreadStore::s_pThreadStore != NULL)
&& ThreadStore::s_pThreadStore->IsCrstForThreadStore(this))
{
return;
}

if (m_dwFlags & CRST_UNSAFE_COOPGC)
{
Expand Down Expand Up @@ -492,8 +493,11 @@ void CrstBase::PostEnter()
}
}

if (ThreadStore::s_pThreadStore->IsCrstForThreadStore(this))
if ((ThreadStore::s_pThreadStore != NULL)
&& ThreadStore::s_pThreadStore->IsCrstForThreadStore(this))
{
return;
}

if (m_dwFlags & (CRST_UNSAFE_ANYMODE | CRST_UNSAFE_COOPGC | CRST_GC_NOTRIGGER_WHEN_TAKEN))
{
Expand Down Expand Up @@ -700,15 +704,13 @@ BOOL CrstBase::IsSafeToTake()
// which case it must always be taken in this mode.
// If there is no thread object, we ignore the check since this thread isn't
// coordinated with the GC.
Thread * pThread;

pThread = GetThreadNULLOk();
Thread * pThread = GetThreadNULLOk();

_ASSERTE(pThread == NULL ||
(pThread->PreemptiveGCDisabled() == ((m_dwFlags & CRST_UNSAFE_COOPGC) != 0)) ||
((m_dwFlags & (CRST_UNSAFE_ANYMODE | CRST_GC_NOTRIGGER_WHEN_TAKEN)) != 0) ||
(GCHeapUtilities::IsGCInProgress() && pThread == ThreadSuspend::GetSuspensionThread()));


if (m_holderthreadid.IsCurrentThread())
{
Expand Down
3 changes: 2 additions & 1 deletion src/coreclr/vm/dllimport.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4119,7 +4119,8 @@ namespace
//
for (int i = 0; i < pParams->m_nParamTokens; ++i)
{
memcpy(pBlobParams, paramInfos[i].pvNativeType, paramInfos[i].cbNativeType);
if (paramInfos[i].cbNativeType > 0)
jkotas marked this conversation as resolved.
Show resolved Hide resolved
memcpy(pBlobParams, paramInfos[i].pvNativeType, paramInfos[i].cbNativeType);
pBlobParams += paramInfos[i].cbNativeType;
}

Expand Down
6 changes: 4 additions & 2 deletions src/coreclr/vm/methodtablebuilder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -396,7 +396,8 @@ MethodTableBuilder::ExpandApproxInterface(
}

bmtInterfaceEntry * pNewMap = (bmtInterfaceEntry *)new (GetStackingAllocator()) BYTE[safeSize.Value()];
memcpy(pNewMap, bmtInterface->pInterfaceMap, sizeof(bmtInterfaceEntry) * bmtInterface->dwInterfaceMapAllocated);
if (bmtInterface->dwInterfaceMapAllocated > 0)
memcpy(pNewMap, bmtInterface->pInterfaceMap, sizeof(bmtInterfaceEntry) * bmtInterface->dwInterfaceMapAllocated);

bmtInterface->pInterfaceMap = pNewMap;
bmtInterface->dwInterfaceMapAllocated = dwNewAllocated.Value();
Expand Down Expand Up @@ -11690,7 +11691,8 @@ void MethodTableBuilder::bmtMethodImplInfo::AddMethodImpl(
// If we have to grow this array, we will not free the old array before we clean up the BuildMethodTable operation
// because this is a stacking allocator. However, the old array will get freed when all the stack allocator is freed.
Entry *rgEntriesNew = new (pStackingAllocator) Entry[newEntriesCount];
memcpy(rgEntriesNew, rgEntries, sizeof(Entry) * cMaxIndex);
if (cMaxIndex > 0)
memcpy(rgEntriesNew, rgEntries, sizeof(Entry) * cMaxIndex);

// Start using newly allocated array.
rgEntries = rgEntriesNew;
Expand Down