[Release 10.0] Support hardware with more than 1024 CPUs - #4
anurag6569201 wants to merge 1 commit into
Conversation
Source PR: dotnet#131740 Source head: 8bb9f3c
⛔ Shipwright · BlockedRecommendation: do not merge PR #4 · Tier
Findings (28)
Fireworks usage: 36,597 input · 3,431 output · 40,028 total tokens · $0.0103 · 27s · 0 fix iteration(s) Open the Shipwright check for full evidence and the audit bundle. Use |
![shipwright-agent[bot]](https://avatars.githubusercontent.com/u/140162738?s=60&v=4){kind=small}
|
|
||
| cpu_set_t* pCpuSet = CPU_ALLOC(g_configuredCpuCount); | ||
| size_t cpuSetSize = CPU_ALLOC_SIZE(g_configuredCpuCount); | ||
| CPU_ZERO_S(cpuSetSize, pCpuSet); |
There was a problem hiding this comment.
Shipwright · CRITICAL
SetThreadAffinity allocates a cpu_set_t sized for g_configuredCpuCount but calls CPU_SET_S with procNo without validating procNo < g_configuredCpuCount.
Impact: SetThreadAffinity allocates a cpu_set_t sized for g_configuredCpuCount but calls CPU_SET_S with procNo without validating procNo < g_configuredCpuCount. A larger procNo causes an out-of-bounds write and memory corruption.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| cpu_set_t cpuSet; | ||
| CPU_ZERO(&cpuSet); | ||
| CPU_SET((int)procNo, &cpuSet); | ||
|
|
There was a problem hiding this comment.
Shipwright · CRITICAL
SetThreadAffinity calls CPU_ALLOC(g_configuredCpuCount) without checking for nullptr before CPU_ZERO_S/CPU_SET_S.
Impact: SetThreadAffinity calls CPU_ALLOC(g_configuredCpuCount) without checking for nullptr before CPU_ZERO_S/CPU_SET_S. Allocation failure dereferences null and crashes.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| AffinitySet(AffinitySet&&) = delete; | ||
| AffinitySet& operator=(AffinitySet&&) = delete; | ||
|
|
||
| bool Initialize(int cpuCount) |
There was a problem hiding this comment.
Shipwright · CRITICAL
AffinitySet::Initialize computes m_bitsetDataSize = (cpuCount + BitsPerBitsetEntry - 1) / BitsPerBitsetEntry without overflow protection.
Impact: AffinitySet::Initialize computes m_bitsetDataSize = (cpuCount + BitsPerBitsetEntry - 1) / BitsPerBitsetEntry without overflow protection. A large uint32_t cpuCount can overflow the addition, causing a small allocation and later out-of-bounds access.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| last_proc_no = proc_no; | ||
| } | ||
|
|
||
| assert(proc_no < GCToOSInterface::GetActiveProcessorCount ()); |
There was a problem hiding this comment.
Shipwright · CRITICAL
The assertion before proc_no_to_heap_no[proc_no] uses GetActiveProcessorCount(), but the array is sized by GetMaxProcessorCount().
Impact: The assertion before proc_no_to_heap_no[proc_no] uses GetActiveProcessorCount(), but the array is sized by GetMaxProcessorCount(). If active count is less than max count, a valid processor index can fail the assertion in debug builds even though the access is safe.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| if (cnt == 0) | ||
| { | ||
| procIndex = i; | ||
| procIndex = (uint16_t)i; |
There was a problem hiding this comment.
Shipwright · CRITICAL
GetProcessorForHeap casts the uint32_t loop index to uint16_t when assigning procIndex.
Impact: GetProcessorForHeap casts the uint32_t loop index to uint16_t when assigning procIndex. On systems with more than 65535 processors this truncates the processor number and can return an incorrect or duplicate processor for a heap.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| last_proc_no = proc_no; | ||
| } | ||
|
|
||
| assert(proc_no < GCToOSInterface::GetActiveProcessorCount ()); |
There was a problem hiding this comment.
Shipwright · HIGH
The assertion 'assert(proc_no < GCToOSInterface::GetActiveProcessorCount ());' is added before indexing 'proc_no_to_heap_no[proc_no]', but the array was allocated with 'GCToOSInter
Impact: The assertion 'assert(proc_no < GCToOSInterface::GetActiveProcessorCount ());' is added before indexing 'proc_no_to_heap_no[proc_no]', but the array was allocated with 'GCToOSInterface::GetMaxProcessorCount()' entries. If 'GetActiveProcessorCount()' can be less than 'GetMaxProcessorCount()' (e.g., due to CPU groups or affinity restrictions), a valid processor number may be >= active count but < max count, c…
Suggested fix: Fix the review finding before release.
| if (cnt == 0) | ||
| { | ||
| procIndex = i; | ||
| procIndex = (uint16_t)i; |
There was a problem hiding this comment.
Shipwright · HIGH
GetProcessorForHeap casts the loop index i (now uint32_t, up to GetTotalProcessorCount()) to uint16_t when assigning procIndex.
Impact: GetProcessorForHeap casts the loop index i (now uint32_t, up to GetTotalProcessorCount()) to uint16_t when assigning procIndex. On systems with more than 65535 processors, this truncates the processor number and can return an incorrect processor for a heap, defeating the >1024 CPU support and potentially assigning duplicate/invalid processor indices.
Suggested fix: Fix the review finding before release.
|
|
||
| cpu_set_t* pCpuSet = CPU_ALLOC(g_configuredCpuCount); | ||
| size_t cpuSetSize = CPU_ALLOC_SIZE(g_configuredCpuCount); | ||
| CPU_ZERO_S(cpuSetSize, pCpuSet); |
There was a problem hiding this comment.
Shipwright · HIGH
SetThreadAffinity allocates a cpu_set_t sized for g_configuredCpuCount but then calls CPU_SET_S with procNo without validating that procNo is less than g_configuredCpuCount.
Impact: SetThreadAffinity allocates a cpu_set_t sized for g_configuredCpuCount but then calls CPU_SET_S with procNo without validating that procNo is less than g_configuredCpuCount. If procNo exceeds the configured count, CPU_SET_S writes out of bounds, corrupting memory. The previous fixed-size cpu_set_t also had this issue for values >= CPU_SETSIZE, but the new dynamic allocation makes the bound explicit and the diff i…
Suggested fix: Fix the review finding before release.
| AffinitySet(AffinitySet&&) = delete; | ||
| AffinitySet& operator=(AffinitySet&&) = delete; | ||
|
|
||
| bool Initialize(int cpuCount) |
There was a problem hiding this comment.
Shipwright · HIGH
'AffinitySet::Initialize' uses 'new (nothrow) uintptr_t[m_bitsetDataSize]' but does not check for integer overflow when computing 'm_bitsetDataSize = (cpuCount + BitsPerBitsetEntry
Impact: 'AffinitySet::Initialize' uses 'new (nothrow) uintptr_t[m_bitsetDataSize]' but does not check for integer overflow when computing 'm_bitsetDataSize = (cpuCount + BitsPerBitsetEntry - 1) / BitsPerBitsetEntry'. If 'cpuCount' is very large (e.g., 'UINT32_MAX'), the addition can overflow, leading to a small allocation and subsequent out-of-bounds access. The caller passes 'GetMaxProcessorCount()', which is a 'uint32_t',…
Suggested fix: Fix the review finding before release.
| } | ||
|
|
||
| if ((start_index >= MAX_SUPPORTED_CPUS) || (end_index >= MAX_SUPPORTED_CPUS) || (end_index < start_index)) | ||
| size_t maxCpuCount = GCToOSInterface::GetMaxProcessorCount(); |
There was a problem hiding this comment.
Shipwright · HIGH
In ParseGCHeapAffinitizeRanges, maxCpuCount is obtained from GCToOSInterface::GetMaxProcessorCount(), which returns g_processAffinitySet.MaxCpuCount().
Impact: In ParseGCHeapAffinitizeRanges, maxCpuCount is obtained from GCToOSInterface::GetMaxProcessorCount(), which returns g_processAffinitySet.MaxCpuCount(). On Windows, g_processAffinitySet is initialized in GCToOSInterface::Initialize() before this parsing path, but the new GetMaxProcessorCount() implementation is only added in gcenv.windows.cpp. Other platforms implementing GCToOSInterface may not provide GetMaxProcess…
Suggested fix: Fix the review finding before release.
| cpu_set_t cpuSet; | ||
| CPU_ZERO(&cpuSet); | ||
| CPU_SET((int)procNo, &cpuSet); | ||
|
|
There was a problem hiding this comment.
Shipwright · HIGH
SetThreadAffinity calls CPU_ALLOC(g_configuredCpuCount) without checking for nullptr before CPU_ZERO_S/CPU_SET_S.
Impact: SetThreadAffinity calls CPU_ALLOC(g_configuredCpuCount) without checking for nullptr before CPU_ZERO_S/CPU_SET_S. On allocation failure this dereferences null and crashes. The Initialize path checks CPU_ALLOC result, but this new allocation does not.
Suggested fix: Fix the review finding before release.
| { | ||
| break; | ||
| } | ||
|
|
There was a problem hiding this comment.
Shipwright · HIGH
minipal_get_cpu_max_possible_count parses /sys/devices/system/cpu/possible with fscanf and fgetc.
Impact: minipal_get_cpu_max_possible_count parses /sys/devices/system/cpu/possible with fscanf and fgetc. If the sysfs list contains spaces around commas, parsing stops prematurely and misses later CPU ranges.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| AffinitySet(const AffinitySet&) = delete; | ||
| AffinitySet& operator=(const AffinitySet&) = delete; | ||
| AffinitySet(AffinitySet&&) = delete; | ||
| AffinitySet& operator=(AffinitySet&&) = delete; |
There was a problem hiding this comment.
Shipwright · HIGH
AffinitySet::Initialize asserts m_bitset == nullptr but has no release-build guard against double initialization.
Impact: AffinitySet::Initialize asserts m_bitset == nullptr but has no release-build guard against double initialization. A second call leaks the first allocation and overwrites the pointer.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| CPU_ZERO_S(cpuSetSize, pCpuSet); | ||
|
|
||
| int st = sched_getaffinity(gPID, cpuSetSize, pCpuSet); | ||
| if (st == 0) |
There was a problem hiding this comment.
Shipwright · HIGH
PAL_GetLogicalCpuCountFromOS recomputes CPU_ALLOC_SIZE(configuredCpuCount) instead of using the already-computed cpuSetSize.
Impact: PAL_GetLogicalCpuCountFromOS recomputes CPU_ALLOC_SIZE(configuredCpuCount) instead of using the already-computed cpuSetSize. This is inconsistent and risks a size mismatch if the value changes between calls.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| AffinitySet(AffinitySet&&) = delete; | ||
| AffinitySet& operator=(AffinitySet&&) = delete; | ||
|
|
||
| bool Initialize(int cpuCount) |
There was a problem hiding this comment.
Shipwright · HIGH
AffinitySet::Initialize does not validate cpuCount > 0.
Impact: AffinitySet::Initialize does not validate cpuCount > 0. A zero cpuCount can produce a zero-sized allocation and later assertions or invalid access in Contains/Add/Remove.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| palError = ERROR_INTERNAL_ERROR; | ||
| goto fail; | ||
| } | ||
| } |
There was a problem hiding this comment.
Shipwright · HIGH
CPalThread::ThreadEntry now treats sched_getaffinity failure as non-fatal and continues without restoring thread affinity.
Impact: CPalThread::ThreadEntry now treats sched_getaffinity failure as non-fatal and continues without restoring thread affinity. This silently skips the affinity restoration required for Snap sandboxing and may allow threads to run with incorrect affinity in constrained environments.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| CPU_ZERO_S(cpuSetSize, pCpuSet); | ||
|
|
||
| int st = sched_getaffinity(gPID, cpuSetSize, pCpuSet); | ||
| if (st == 0) |
There was a problem hiding this comment.
Shipwright · MEDIUM
PAL_GetLogicalCpuCountFromOS calls CPU_COUNT_S(CPU_ALLOC_SIZE(configuredCpuCount), pCpuSet) instead of using the already-computed cpuSetSize variable.
Impact: PAL_GetLogicalCpuCountFromOS calls CPU_COUNT_S(CPU_ALLOC_SIZE(configuredCpuCount), pCpuSet) instead of using the already-computed cpuSetSize variable. This is functionally equivalent but recomputes the allocation size; more importantly, if CPU_ALLOC_SIZE is not idempotent or the configuredCpuCount changes between calls, the size could differ from the buffer allocated. The local cpuSetSize should be used for consiste…
Suggested fix: Fix the review finding before release.
| return g_totalCpuCount; | ||
| } | ||
|
|
||
| uint32_t GCToOSInterface::GetMaxProcessorCount() |
There was a problem hiding this comment.
Shipwright · MEDIUM
The new GetMaxProcessorCount returns g_processAffinitySet.MaxCpuCount(), which is the maximum possible CPU count, not the total processor count.
Impact: The new GetMaxProcessorCount returns g_processAffinitySet.MaxCpuCount(), which is the maximum possible CPU count, not the total processor count. If callers expect the number of processors available to the process, this can over-report CPUs on systems where the affinity set is sparse or configured CPU count exceeds online CPUs. The diff does not show callers, but the method name and existing GetTotalProcessorCount di…
Suggested fix: Fix the review finding before release.
| AffinitySet(AffinitySet&&) = delete; | ||
| AffinitySet& operator=(AffinitySet&&) = delete; | ||
|
|
||
| bool Initialize(int cpuCount) |
There was a problem hiding this comment.
Shipwright · MEDIUM
'AffinitySet::Initialize' does not handle the case where 'cpuCount' is zero.
Impact: 'AffinitySet::Initialize' does not handle the case where 'cpuCount' is zero. If 'cpuCount' is zero, 'm_bitsetDataSize' becomes zero, and 'new (nothrow) uintptr_t[0]' may return a non-null pointer or null depending on the implementation. If it returns non-null, 'memset' with size zero is fine, but subsequent 'Contains'/'Add'/'Remove' calls with any index will assert because 'GetBitsetEntryIndex(cpuIndex) < m_bitse…
Suggested fix: Fix the review finding before release.
| palError = ERROR_INTERNAL_ERROR; | ||
| goto fail; | ||
| } | ||
| } |
There was a problem hiding this comment.
Shipwright · MEDIUM
In CPalThread::ThreadEntry, when sched_getaffinity fails, the code now treats it as non-fatal and continues without setting the thread affinity.
Impact: In CPalThread::ThreadEntry, when sched_getaffinity fails, the code now treats it as non-fatal and continues without setting the thread affinity. The previous behavior returned ERROR_INTERNAL_ERROR and aborted thread creation. This change silently skips the affinity restoration that the surrounding comment says is required for Snap sandboxing, potentially allowing threads to run with incorrect affinity in constrained…
Suggested fix: Fix the review finding before release.
| { | ||
| break; | ||
| } | ||
|
|
There was a problem hiding this comment.
Shipwright · MEDIUM
'minipal_get_cpu_max_possible_count' uses 'fscanf' to parse the possible CPU list.
Impact: 'minipal_get_cpu_max_possible_count' uses 'fscanf' to parse the possible CPU list. The format '%d-%d' does not handle whitespace or newlines robustly. If the file contains a trailing newline after the last range, the loop will break after reading the last range and then 'fgetc' will return the newline, which is not '','', so the loop breaks. This is correct. However, if the file contains spaces around the comma (e.g…
Suggested fix: Fix the review finding before release.
| AffinitySet(const AffinitySet&) = delete; | ||
| AffinitySet& operator=(const AffinitySet&) = delete; | ||
| AffinitySet(AffinitySet&&) = delete; | ||
| AffinitySet& operator=(AffinitySet&&) = delete; |
There was a problem hiding this comment.
Shipwright · MEDIUM
'AffinitySet::Initialize' asserts 'm_bitset == nullptr' but does not prevent double initialization in release builds.
Impact: 'AffinitySet::Initialize' asserts 'm_bitset == nullptr' but does not prevent double initialization in release builds. If 'Initialize' is called twice, the second call will leak the first allocation and overwrite the pointer. The diff does not show any guard against this in release builds.
Suggested fix: Fix the review finding before release.
| if (maxCpu != -1) | ||
| { | ||
| return maxCpu + 1; | ||
| } |
There was a problem hiding this comment.
Shipwright · LOW
'minipal_get_cpu_max_possible_count' returns the maximum CPU index plus one, but the caller uses this value as the CPU count for 'CPU_ALLOC'.
Impact: 'minipal_get_cpu_max_possible_count' returns the maximum CPU index plus one, but the caller uses this value as the CPU count for 'CPU_ALLOC'. On Linux, 'CPU_ALLOC' expects the number of CPUs, so this is correct. However, the fallback 'sysconf(_SC_NPROCESSORS_CONF)' returns the number of configured processors, which is also a count. The issue is that the function name and documentation imply a count, but the parsing…
Suggested fix: Fix the review finding before release.
| { | ||
| size_t count = 0; | ||
| for (size_t i = 0; i < MAX_SUPPORTED_CPUS; i++) | ||
| for (size_t i = 0; i < m_bitsetDataSize * BitsPerBitsetEntry; i++) |
There was a problem hiding this comment.
Shipwright · LOW
'AffinitySet::Count' iterates up to 'm_bitsetDataSize * BitsPerBitsetEntry' and calls 'Contains(i)' for each index.
Impact: 'AffinitySet::Count' iterates up to 'm_bitsetDataSize * BitsPerBitsetEntry' and calls 'Contains(i)' for each index. 'Contains' asserts that 'GetBitsetEntryIndex(cpuIndex) < m_bitsetDataSize'. For the last valid index, 'GetBitsetEntryIndex' returns 'm_bitsetDataSize - 1', so the assert passes. However, if 'm_bitsetDataSize' is zero (e.g., 'Initialize' was never called or failed), the loop does not execute, so no a…
Suggested fix: Fix the review finding before release.
| } | ||
|
|
||
| count = CPU_COUNT(&cpuSet); | ||
| if (count == 0) |
There was a problem hiding this comment.
Shipwright · LOW
In 'InitializeCurrentProcessCpuCount', the fallback to 'GCToOSInterface::GetTotalProcessorCount()' is used when 'count == 0'.
Impact: In 'InitializeCurrentProcessCpuCount', the fallback to 'GCToOSInterface::GetTotalProcessorCount()' is used when 'count == 0'. However, 'count' is initialized to 0 and only set if 'sched_getaffinity' succeeds. If 'sched_getaffinity' succeeds but returns 0 CPUs (which is possible if the process has no affinity to any CPU), the fallback is triggered, which may be intentional. This is not a defect.
Suggested fix: Fix the review finding before release.
| @@ -0,0 +1,49 @@ | |||
| // Licensed to the .NET Foundation under one or more agreements. | |||
There was a problem hiding this comment.
Shipwright · LOW
The new file 'src/native/minipal/cpucount.c' includes '<stdio.h>' and '<unistd.h>' but the header 'cpucount.h' is not shown in the diff.
Impact: The new file 'src/native/minipal/cpucount.c' includes '<stdio.h>' and '<unistd.h>' but the header 'cpucount.h' is not shown in the diff. If the header does not declare 'minipal_get_cpu_max_possible_count' with the correct calling convention or visibility, this could cause a linker error. However, the diff does not show the header, so this cannot be confirmed.
Suggested fix: Fix the review finding before release.
| #ifndef __GCENV_OS_H__ | ||
| #define __GCENV_OS_H__ | ||
|
|
||
| #include <new> |
There was a problem hiding this comment.
Shipwright · LOW
'AffinitySet::Initialize' uses 'new (nothrow)' but does not include '<new>' in the file where it is used.
Impact: 'AffinitySet::Initialize' uses 'new (nothrow)' but does not include '<new>' in the file where it is used. The diff adds '#include <new>' at the top of 'gcenv.os.h', so this is not a defect.
Suggested fix: Fix the review finding before release.
| ~AffinitySet() | ||
| { | ||
| memset(m_bitset, 0, sizeof(m_bitset)); | ||
| delete[] m_bitset; |
There was a problem hiding this comment.
Shipwright · LOW
The 'AffinitySet' destructor calls 'delete[] m_bitset' without checking for null.
Impact: The 'AffinitySet' destructor calls 'delete[] m_bitset' without checking for null. 'delete[]' on a null pointer is safe, so this is not a defect.
Suggested fix: Fix the review finding before release.
1 participant
Backport of dotnet#126763 and dotnet#127572 to release/10.0
Customer Impact
.NET runtime fails to initialize on Linux on machines that have more than 1024 CPUs due to
sched_getaffinitybeing passed the default instance ofcpu_set_tthat supports max 1024 CPUs and fails if there are more CPUs on the current machine.This occurs also in case of containers limited to a small number of CPUs running on a host with more than 1024 CPUs. So it is impossible to use .NET on such machines.
Regression
Testing
Directed test on Azure VM with 1792 CPU cores, CI testing coreclr and libraries tests.
Risk
Low. The change has been in main since the beginning of April and no issues were discovered.
Source merge-base:
76ca66ef3bed7037ac2f7f708d48baf93953a8d6Source head:
8bb9f3c49f7350fe8806d489c0476ac90a35e42f