From 1a1b1f8d8ec592e669de3f2681cc0cbc853f7d96 Mon Sep 17 00:00:00 2001 From: Trung Nguyen <57174311+trungnt2910@users.noreply.github.com> Date: Thu, 8 Jun 2023 08:26:37 +1000 Subject: [PATCH] [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. --- src/coreclr/gc/gc.cpp | 34 +++++++++++++++++----------------- src/coreclr/gc/gcdesc.h | 2 +- src/coreclr/vm/array.cpp | 5 ++++- src/coreclr/vm/i386/stublinkerx86.cpp | 4 ++-- 4 files changed, 24 insertions(+), 21 deletions(-) diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp index 461d1f2..453cff7 100644 --- a/src/coreclr/gc/gc.cpp +++ b/src/coreclr/gc/gc.cpp @@ -7841,7 +7841,7 @@ void gc_heap::fix_allocation_context_heaps (gc_alloc_context* gc_context, void*) // make sure no allocation contexts point to idle heaps void gc_heap::fix_allocation_contexts_heaps() { - GCToEEInterface::GcEnumAllocContexts (fix_allocation_context_heaps, nullptr); + GCToEEInterface::GcEnumAllocContexts (fix_allocation_context_heaps, nullptr); } #endif //MULTIPLE_HEAPS @@ -15851,9 +15851,9 @@ void allocator::count_items (gc_heap* this_hp, size_t* fl_items_count, size_t* f assert (((CObjectHeader*)free_item)->IsFree()); num_fl_items++; - // Get the heap its region belongs to see if we need to put it back. + // Get the heap its region belongs to see if we need to put it back. heap_segment* region = gc_heap::region_of (free_item); - dprintf (3, ("b#%2d FL %Ix region %Ix heap %d -> %d", + dprintf (3, ("b#%2d FL %Ix region %Ix heap %d -> %d", i, free_item, (size_t)region, this_hp->heap_number, region->heap->heap_number)); if (region->heap != this_hp) { @@ -15862,7 +15862,7 @@ void allocator::count_items (gc_heap* this_hp, size_t* fl_items_count, size_t* f //if ((num_fl_items_rethread % 1000) == 0) //{ // end_us = GetHighPrecisionTimeStamp(); - // dprintf (8888, ("%Id items rethreaded out of %Id items in %I64d us, current fl: %Ix", + // dprintf (8888, ("%Id items rethreaded out of %Id items in %I64d us, current fl: %Ix", // num_fl_items_rethread, num_fl_items, (end_us - start_us), free_item)); // start_us = end_us; //} @@ -15954,9 +15954,9 @@ void allocator::rethread_items (size_t* num_total_fl_items, size_t* num_total_fl assert (((CObjectHeader*)free_item)->IsFree()); num_fl_items++; - // Get the heap its region belongs to see if we need to put it back. + // Get the heap its region belongs to see if we need to put it back. heap_segment* region = gc_heap::region_of (free_item); - dprintf (3, ("b#%2d FL %Ix region %Ix heap %d -> %d", + dprintf (3, ("b#%2d FL %Ix region %Ix heap %d -> %d", i, free_item, (size_t)region, current_heap->heap_number, region->heap->heap_number)); // need to keep track of heap and only check if it's not from our heap!! if (region->heap != current_heap) @@ -16000,7 +16000,7 @@ void allocator::rethread_items (size_t* num_total_fl_items, size_t* num_total_fl // merge buckets from min_fl_list to their corresponding buckets to this FL. void allocator::merge_items (gc_heap* current_heap, int to_num_heaps, int from_num_heaps) { - int this_hn = current_heap->heap_number; + int this_hn = current_heap->heap_number; for (unsigned int i = 0; i < num_buckets; i++) { @@ -22495,7 +22495,7 @@ void gc_heap::gc1() bool gc_heap::prepare_rethread_fl_items() { if (!min_fl_list) - { + { min_fl_list = new (nothrow) min_fl_list_info [MAX_BUCKET_COUNT * n_max_heaps]; if (min_fl_list == nullptr) return false; @@ -24899,7 +24899,7 @@ void gc_heap::check_heap_count () if (GCConfig::GetGCDynamicAdaptationMode() == 0) { // don't change the heap count dynamically if the feature isn't explicitly enabled - return; + return; } if (heap_number == 0) @@ -25889,8 +25889,8 @@ BOOL gc_heap::background_mark (uint8_t* o, uint8_t* low, uint8_t* high) { \ for (ptrdiff_t __i = 0; __i > cnt; __i--) \ { \ - HALF_SIZE_T skip = cur->val_serie[__i].skip; \ - HALF_SIZE_T nptrs = cur->val_serie[__i].nptrs; \ + HALF_SIZE_T skip = (cur->val_serie + __i)->skip; \ + HALF_SIZE_T nptrs = (cur->val_serie + __i)->nptrs; \ uint8_t** ppstop = parm + nptrs; \ if (!start_useful || (uint8_t*)ppstop > (start)) \ { \ @@ -30961,21 +30961,21 @@ void gc_heap::process_remaining_regions (int current_plan_gen_num, generation* c // // + if the pinned surv of a region is >= demotion_pinned_ratio_th (this will be dynamically tuned based on memory load), // it will be promoted to its normal planned generation unconditionally. - // + // // + if the pinned surv is < demotion_pinned_ratio_th, we will always demote it to gen0. We will record how many regions // have no survival at all - those will be empty and can be used to plan any non gen0 generation if needed. - // + // // Note! We could actually promote a region with non zero pinned survivors to whichever generation we'd like (eg, we could // 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 // 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 // have planned regions in higher generations. So I don't think it's worth the complexicity for now. We may consider it // for the future. - // + // // + if after we are done walking the remaining regions, we still haven't successfully planned all the needed generations, // 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 // these regions which means they are planned in gen0. So we need to make sure at least gen0 has 1 region). If so // thread_final_regions will naturally get one from there so we don't need to call set_region_plan_gen_num to replace the - // plan gen num. + // plan gen num. // // + 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 // to the special sweep mode. @@ -31026,7 +31026,7 @@ void gc_heap::process_remaining_regions (int current_plan_gen_num, generation* c heap_segment_plan_allocated (nseg) = generation_allocation_pointer (consing_gen); decide_on_demotion_pin_surv (nseg, &to_be_empty_regions); - + heap_segment* next_seg = heap_segment_next_non_sip (nseg); if ((next_seg == 0) && (heap_segment_gen_num (nseg) > 0)) @@ -46231,7 +46231,7 @@ void gc_heap::descr_generations (const char* msg) 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", msg, idx, alloc_size, frag_size, (int)((double)frag_size * 100.0 / (double)alloc_size), - commit_size, + commit_size, (double)elapsed_time_so_far / (double)1000000 / (double)60, total_new_gen0_regions_in_plns, total_new_regions_in_prr, total_new_regions_in_threading)); } diff --git a/src/coreclr/gc/gcdesc.h b/src/coreclr/gc/gcdesc.h index 14cee1d..54a13df 100644 --- a/src/coreclr/gc/gcdesc.h +++ b/src/coreclr/gc/gcdesc.h @@ -216,7 +216,7 @@ public: /* Handle the repeating case - array of valuetypes */ for (ptrdiff_t __i = 0; __i > cnt; __i--) { - NumOfPointers += cur->val_serie[__i].nptrs; + NumOfPointers += (cur->val_serie + __i)->nptrs; } NumOfPointers *= NumComponents; diff --git a/src/coreclr/vm/array.cpp b/src/coreclr/vm/array.cpp index 54ecd0d..aaa2034 100644 --- a/src/coreclr/vm/array.cpp +++ b/src/coreclr/vm/array.cpp @@ -665,7 +665,10 @@ MethodTable* Module::CreateArrayMethodTable(TypeHandle elemTypeHnd, CorElementTy IDS_CLASSLOAD_VALUECLASSTOOLARGE); } - val_serie_item *val_item = &(pSeries->val_serie[-index]); + // pSeries->val_serie is a fixed sized array. + // 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. + val_serie_item *val_item = pSeries->val_serie - index; val_item->set_val_serie_item (NumPtrs, (unsigned short)skip); } diff --git a/src/coreclr/vm/i386/stublinkerx86.cpp b/src/coreclr/vm/i386/stublinkerx86.cpp index 3203e4f..fde6801 100644 --- a/src/coreclr/vm/i386/stublinkerx86.cpp +++ b/src/coreclr/vm/i386/stublinkerx86.cpp @@ -4558,8 +4558,8 @@ COPY_VALUE_CLASS: for (SSIZE_T __i = 0; __i > cnt; __i--) { - HALF_SIZE_T skip = cur->val_serie[__i].skip; - HALF_SIZE_T nptrs = cur->val_serie[__i].nptrs; + HALF_SIZE_T skip = (cur->val_serie + __i)->skip; + HALF_SIZE_T nptrs = (cur->val_serie + __i)->nptrs; total += nptrs*sizeof (Object*); do { -- 2.7.4