Skip to content

Commit 1a1b1f8

Browse files
authored
[VM] Fix potential undefined behavior (#87119)
Use pointer arithmetic instead of direct array access to avoid compilers, specifically GCC, to discover undefined behavior and generate unintended code when optimization is turned on. The array involved is `pSeries->val_serie`, which is declared as a fixed sized array of size 1. However, `index` is always a non-negative integer, and we want to access the memory at `-index`, which is either zero or negative.
1 parent c236ec0 commit 1a1b1f8

File tree

4 files changed

+24
-21
lines changed

4 files changed

+24
-21
lines changed

src/coreclr/gc/gc.cpp

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -7841,7 +7841,7 @@ void gc_heap::fix_allocation_context_heaps (gc_alloc_context* gc_context, void*)
78417841
// make sure no allocation contexts point to idle heaps
78427842
void gc_heap::fix_allocation_contexts_heaps()
78437843
{
7844-
GCToEEInterface::GcEnumAllocContexts (fix_allocation_context_heaps, nullptr);
7844+
GCToEEInterface::GcEnumAllocContexts (fix_allocation_context_heaps, nullptr);
78457845
}
78467846
#endif //MULTIPLE_HEAPS
78477847

@@ -15851,9 +15851,9 @@ void allocator::count_items (gc_heap* this_hp, size_t* fl_items_count, size_t* f
1585115851
assert (((CObjectHeader*)free_item)->IsFree());
1585215852

1585315853
num_fl_items++;
15854-
// Get the heap its region belongs to see if we need to put it back.
15854+
// Get the heap its region belongs to see if we need to put it back.
1585515855
heap_segment* region = gc_heap::region_of (free_item);
15856-
dprintf (3, ("b#%2d FL %Ix region %Ix heap %d -> %d",
15856+
dprintf (3, ("b#%2d FL %Ix region %Ix heap %d -> %d",
1585715857
i, free_item, (size_t)region, this_hp->heap_number, region->heap->heap_number));
1585815858
if (region->heap != this_hp)
1585915859
{
@@ -15862,7 +15862,7 @@ void allocator::count_items (gc_heap* this_hp, size_t* fl_items_count, size_t* f
1586215862
//if ((num_fl_items_rethread % 1000) == 0)
1586315863
//{
1586415864
// end_us = GetHighPrecisionTimeStamp();
15865-
// dprintf (8888, ("%Id items rethreaded out of %Id items in %I64d us, current fl: %Ix",
15865+
// dprintf (8888, ("%Id items rethreaded out of %Id items in %I64d us, current fl: %Ix",
1586615866
// num_fl_items_rethread, num_fl_items, (end_us - start_us), free_item));
1586715867
// start_us = end_us;
1586815868
//}
@@ -15954,9 +15954,9 @@ void allocator::rethread_items (size_t* num_total_fl_items, size_t* num_total_fl
1595415954
assert (((CObjectHeader*)free_item)->IsFree());
1595515955

1595615956
num_fl_items++;
15957-
// Get the heap its region belongs to see if we need to put it back.
15957+
// Get the heap its region belongs to see if we need to put it back.
1595815958
heap_segment* region = gc_heap::region_of (free_item);
15959-
dprintf (3, ("b#%2d FL %Ix region %Ix heap %d -> %d",
15959+
dprintf (3, ("b#%2d FL %Ix region %Ix heap %d -> %d",
1596015960
i, free_item, (size_t)region, current_heap->heap_number, region->heap->heap_number));
1596115961
// need to keep track of heap and only check if it's not from our heap!!
1596215962
if (region->heap != current_heap)
@@ -16000,7 +16000,7 @@ void allocator::rethread_items (size_t* num_total_fl_items, size_t* num_total_fl
1600016000
// merge buckets from min_fl_list to their corresponding buckets to this FL.
1600116001
void allocator::merge_items (gc_heap* current_heap, int to_num_heaps, int from_num_heaps)
1600216002
{
16003-
int this_hn = current_heap->heap_number;
16003+
int this_hn = current_heap->heap_number;
1600416004

1600516005
for (unsigned int i = 0; i < num_buckets; i++)
1600616006
{
@@ -22495,7 +22495,7 @@ void gc_heap::gc1()
2249522495
bool gc_heap::prepare_rethread_fl_items()
2249622496
{
2249722497
if (!min_fl_list)
22498-
{
22498+
{
2249922499
min_fl_list = new (nothrow) min_fl_list_info [MAX_BUCKET_COUNT * n_max_heaps];
2250022500
if (min_fl_list == nullptr)
2250122501
return false;
@@ -24899,7 +24899,7 @@ void gc_heap::check_heap_count ()
2489924899
if (GCConfig::GetGCDynamicAdaptationMode() == 0)
2490024900
{
2490124901
// don't change the heap count dynamically if the feature isn't explicitly enabled
24902-
return;
24902+
return;
2490324903
}
2490424904

2490524905
if (heap_number == 0)
@@ -25889,8 +25889,8 @@ BOOL gc_heap::background_mark (uint8_t* o, uint8_t* low, uint8_t* high)
2588925889
{ \
2589025890
for (ptrdiff_t __i = 0; __i > cnt; __i--) \
2589125891
{ \
25892-
HALF_SIZE_T skip = cur->val_serie[__i].skip; \
25893-
HALF_SIZE_T nptrs = cur->val_serie[__i].nptrs; \
25892+
HALF_SIZE_T skip = (cur->val_serie + __i)->skip; \
25893+
HALF_SIZE_T nptrs = (cur->val_serie + __i)->nptrs; \
2589425894
uint8_t** ppstop = parm + nptrs; \
2589525895
if (!start_useful || (uint8_t*)ppstop > (start)) \
2589625896
{ \
@@ -30961,21 +30961,21 @@ void gc_heap::process_remaining_regions (int current_plan_gen_num, generation* c
3096130961
//
3096230962
// + if the pinned surv of a region is >= demotion_pinned_ratio_th (this will be dynamically tuned based on memory load),
3096330963
// it will be promoted to its normal planned generation unconditionally.
30964-
//
30964+
//
3096530965
// + if the pinned surv is < demotion_pinned_ratio_th, we will always demote it to gen0. We will record how many regions
3096630966
// have no survival at all - those will be empty and can be used to plan any non gen0 generation if needed.
30967-
//
30967+
//
3096830968
// Note! We could actually promote a region with non zero pinned survivors to whichever generation we'd like (eg, we could
3096930969
// promote a gen0 region to gen2). However it means we'd need to set cards on those objects because we will not have a chance
3097030970
// later. The benefit of doing this is small in general as when we get into this method, it's very rare we don't already
3097130971
// have planned regions in higher generations. So I don't think it's worth the complexicity for now. We may consider it
3097230972
// for the future.
30973-
//
30973+
//
3097430974
// + if after we are done walking the remaining regions, we still haven't successfully planned all the needed generations,
3097530975
// we check to see if we have enough in the regions that will be empty (note that we call set_region_plan_gen_num on
3097630976
// these regions which means they are planned in gen0. So we need to make sure at least gen0 has 1 region). If so
3097730977
// thread_final_regions will naturally get one from there so we don't need to call set_region_plan_gen_num to replace the
30978-
// plan gen num.
30978+
// plan gen num.
3097930979
//
3098030980
// + if we don't have enough in regions that will be empty, we'll need to ask for new regions and if we can't, we fall back
3098130981
// to the special sweep mode.
@@ -31026,7 +31026,7 @@ void gc_heap::process_remaining_regions (int current_plan_gen_num, generation* c
3102631026

3102731027
heap_segment_plan_allocated (nseg) = generation_allocation_pointer (consing_gen);
3102831028
decide_on_demotion_pin_surv (nseg, &to_be_empty_regions);
31029-
31029+
3103031030
heap_segment* next_seg = heap_segment_next_non_sip (nseg);
3103131031

3103231032
if ((next_seg == 0) && (heap_segment_gen_num (nseg) > 0))
@@ -46231,7 +46231,7 @@ void gc_heap::descr_generations (const char* msg)
4623146231
dprintf (1, ("[%5s] GC#%5Id total heap size: %Idmb (F: %Idmb %d%%) commit size: %Idmb, %0.3f min, %d,%d new in plan, %d in threading\n",
4623246232
msg, idx, alloc_size, frag_size,
4623346233
(int)((double)frag_size * 100.0 / (double)alloc_size),
46234-
commit_size,
46234+
commit_size,
4623546235
(double)elapsed_time_so_far / (double)1000000 / (double)60,
4623646236
total_new_gen0_regions_in_plns, total_new_regions_in_prr, total_new_regions_in_threading));
4623746237
}

src/coreclr/gc/gcdesc.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -216,7 +216,7 @@ class CGCDesc
216216
/* Handle the repeating case - array of valuetypes */
217217
for (ptrdiff_t __i = 0; __i > cnt; __i--)
218218
{
219-
NumOfPointers += cur->val_serie[__i].nptrs;
219+
NumOfPointers += (cur->val_serie + __i)->nptrs;
220220
}
221221

222222
NumOfPointers *= NumComponents;

src/coreclr/vm/array.cpp

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -665,7 +665,10 @@ MethodTable* Module::CreateArrayMethodTable(TypeHandle elemTypeHnd, CorElementTy
665665
IDS_CLASSLOAD_VALUECLASSTOOLARGE);
666666
}
667667

668-
val_serie_item *val_item = &(pSeries->val_serie[-index]);
668+
// pSeries->val_serie is a fixed sized array.
669+
// Use pointer arithmetic instead of direct array access to avoid compilers, specifically GCC,
670+
// to discover undefined behavior and generate unintended code when optimization is turned on.
671+
val_serie_item *val_item = pSeries->val_serie - index;
669672

670673
val_item->set_val_serie_item (NumPtrs, (unsigned short)skip);
671674
}

src/coreclr/vm/i386/stublinkerx86.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4558,8 +4558,8 @@ VOID StubLinkerCPU::EmitArrayOpStub(const ArrayOpScript* pArrayOpScript)
45584558

45594559
for (SSIZE_T __i = 0; __i > cnt; __i--)
45604560
{
4561-
HALF_SIZE_T skip = cur->val_serie[__i].skip;
4562-
HALF_SIZE_T nptrs = cur->val_serie[__i].nptrs;
4561+
HALF_SIZE_T skip = (cur->val_serie + __i)->skip;
4562+
HALF_SIZE_T nptrs = (cur->val_serie + __i)->nptrs;
45634563
total += nptrs*sizeof (Object*);
45644564
do
45654565
{

0 commit comments

Comments
 (0)