[VM] Fix potential undefined behavior

[trungnt2910]

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.

This fixes some bugs that occur only in Release mode such as this one.

[trungnt2910]

A way to reproduce this bug is to compile the following extracts of CoreCLR code on GCC with -O2:

shared.h

#pragma once

#include <stddef.h>
#include <stdint.h>
#include <assert.h>

#define _ASSERTE assert

typedef uint32_t HALF_SIZE_T;
typedef size_t *JSlot;
typedef int BOOL;
typedef size_t* PTR_size_t;
typedef uint8_t* PTR_uint8_t;
typedef long SSIZE_T;
typedef unsigned int DWORD; // NOTE: diff from  windows.h, for LP64 compat
typedef unsigned short WORD;
typedef uint64_t ULONG_PTR;
typedef ULONG_PTR TADDR;

#define FALSE 0

typedef void* PTR_Module;
typedef void* PTR_MethodTableWriteableData;


typedef class MethodTable* PTR_MethodTable;
class MethodTable
{
public:
        // Low WORD is component size for array and string types (HasComponentSize() returns true).
    // Used for flags otherwise.
    DWORD           m_dwFlags;

    // Base size of instance of this class when allocated on the heap
    DWORD           m_BaseSize;

    // See WFLAGS2_ENUM for values.
    WORD            m_wFlags2;

    // Class token if it fits into 16-bits. If this is (WORD)-1, the class token is stored in the TokenOverflow optional member.
    WORD            m_wToken;

    // <NICE> In the normal cases we shouldn't need a full word for each of these </NICE>
    WORD            m_wNumVirtuals;
    WORD            m_wNumInterfaces;

    PTR_MethodTable m_pParentMethodTable;

    PTR_Module      m_pLoaderModule;

    PTR_MethodTableWriteableData m_pWriteableData;

        TADDR m_pCanonMT;
        TADDR         m_pMultipurposeSlot1;
        TADDR               m_pMultipurposeSlot2;

    enum WFLAGS_HIGH_ENUM
    {
        // DO NOT use flags that have bits set in the low 2 bytes.
        // These flags are DWORD sized so that our atomic masking
        // operations can operate on the entire 4-byte aligned DWORD
        // instead of the logical non-aligned WORD of flags.  The
        // low WORD of flags is reserved for the component size.

        // The following bits describe mutually exclusive locations of the type
        // in the type hierarchy.
        enum_flag_Category_Mask             = 0x000F0000,

        enum_flag_Category_Class            = 0x00000000,
        enum_flag_Category_Unused_1         = 0x00010000,
        enum_flag_Category_Unused_2         = 0x00020000,
        enum_flag_Category_Unused_3         = 0x00030000,

        enum_flag_Category_ValueType        = 0x00040000,
        enum_flag_Category_ValueType_Mask   = 0x000C0000,
        enum_flag_Category_Nullable         = 0x00050000, // sub-category of ValueType
        enum_flag_Category_PrimitiveValueType=0x00060000, // sub-category of ValueType, Enum or primitive value type
        enum_flag_Category_TruePrimitive    = 0x00070000, // sub-category of ValueType, Primitive (ELEMENT_TYPE_I, etc.)

        enum_flag_Category_Array            = 0x00080000,
        enum_flag_Category_Array_Mask       = 0x000C0000,
        // enum_flag_Category_IfArrayThenUnused                 = 0x00010000, // sub-category of Array
        enum_flag_Category_IfArrayThenSzArray                   = 0x00020000, // sub-category of Array

        enum_flag_Category_Interface        = 0x000C0000,
        enum_flag_Category_Unused_4         = 0x000D0000,
        enum_flag_Category_Unused_5         = 0x000E0000,
        enum_flag_Category_Unused_6         = 0x000F0000,

        enum_flag_Category_ElementTypeMask  = 0x000E0000, // bits that matter for element type mask


        enum_flag_HasFinalizer                = 0x00100000, // instances require finalization

        enum_flag_IDynamicInterfaceCastable   = 0x00200000, // class implements IDynamicInterfaceCastable interface

        enum_flag_ICastable                   = 0x00400000, // class implements ICastable interface

        enum_flag_Unused_1                    = 0x00800000,

        enum_flag_ContainsPointers            = 0x01000000,

        enum_flag_HasTypeEquivalence          = 0x02000000, // can be equivalent to another type

        enum_flag_IsTrackedReferenceWithFinalizer   = 0x04000000,

        enum_flag_HasCriticalFinalizer        = 0x08000000, // finalizer must be run on Appdomain Unload
        enum_flag_Collectible                 = 0x10000000,
        enum_flag_ContainsGenericVariables    = 0x20000000,   // we cache this flag to help detect these efficiently and
                                                              // to detect this condition when restoring

        enum_flag_ComObject                   = 0x40000000, // class is a com object

        enum_flag_HasComponentSize            = 0x80000000,   // This is set if component size is used for flags.

        // Types that require non-trivial interface cast have this bit set in the category
        enum_flag_NonTrivialInterfaceCast   =  enum_flag_Category_Array
                                             | enum_flag_ComObject
                                             | enum_flag_ICastable
                                             | enum_flag_IDynamicInterfaceCastable
                                             | enum_flag_Category_ValueType

    };  // enum WFLAGS_HIGH_ENUM
public:
    DWORD GetFlag(WFLAGS_HIGH_ENUM flag) const
    {
        return m_dwFlags & flag;
    }


    DWORD           GetBaseSize()
    {
        return(m_BaseSize);
    }

    DWORD           ContainsPointers()
    {
        return GetFlag(enum_flag_ContainsPointers);
    }

    BOOL            Collectible()
    {
#ifdef FEATURE_COLLECTIBLE_TYPES
        return GetFlag(enum_flag_Collectible);
#else
        return FALSE;
#endif
    }

    inline DWORD GetNumInstanceFieldBytes()
    {
        return(GetBaseSize() /*- GetClass()->GetBaseSizePadding() */);
    }

};

struct val_serie_item
{
    HALF_SIZE_T nptrs;
    HALF_SIZE_T skip;
    void set_val_serie_item (HALF_SIZE_T nptrs, HALF_SIZE_T skip)
    {
        this->nptrs = nptrs;
        this->skip = skip;
    }
};

typedef class CGCDescSeries* PTR_CGCDescSeries;
class CGCDescSeries
{
public:
    union
    {
        size_t seriessize;              // adjusted length of series (see above) in bytes
        val_serie_item val_serie[1];    //coded serie for value class array
    };

    size_t startoffset;

    size_t GetSeriesCount ()
    {
        return seriessize/sizeof(JSlot);
    }

    void SetSeriesCount (size_t newcount)
    {
        seriessize = newcount * sizeof(JSlot);
    }

    void IncSeriesCount (size_t increment = 1)
    {
        seriessize += increment * sizeof(JSlot);
    }

    size_t GetSeriesSize ()
    {
        return seriessize;
    }

    void SetSeriesSize (size_t newsize)
    {
        seriessize = newsize;
    }

    void SetSeriesValItem (val_serie_item item, int index)
    {
        val_serie [index] = item;
    }

    void SetSeriesOffset (size_t newoffset)
    {
        startoffset = newoffset;
    }

    size_t GetSeriesOffset ()
    {
        return startoffset;
    }
};

typedef class CGCDesc* PTR_CGCDesc;
class CGCDesc
{
    // Don't construct me, you have to hand me a ptr to the *top* of my storage in Init.
    CGCDesc () {}

    //
    // NOTE: for alignment reasons, NumSeries is stored as a size_t.
    //       This makes everything nicely 8-byte aligned on IA64.
    //
public:
    static size_t ComputeSize (size_t NumSeries)
    {
        _ASSERTE (ptrdiff_t(NumSeries) > 0);

        return sizeof(size_t) + NumSeries*sizeof(CGCDescSeries);
    }

    // For value type array
    static size_t ComputeSizeRepeating (size_t NumSeries)
    {
        _ASSERTE (ptrdiff_t(NumSeries) > 0);

        return sizeof(size_t) + sizeof(CGCDescSeries) +
               (NumSeries-1)*sizeof(val_serie_item);
    }

#ifndef DACCESS_COMPILE
    static void Init (void* mem, size_t NumSeries)
    {
        *((size_t*)mem-1) = NumSeries;
    }

    static void InitValueClassSeries (void* mem, size_t NumSeries)
    {
        *((ptrdiff_t*)mem-1) = -((ptrdiff_t)NumSeries);
    }
#endif

    static PTR_CGCDesc GetCGCDescFromMT (MethodTable * pMT)
    {
        // If it doesn't contain pointers, there isn't a GCDesc
        PTR_MethodTable mt(pMT);

        _ASSERTE(mt->ContainsPointers());

        return PTR_CGCDesc(mt);
    }

    size_t GetNumSeries ()
    {
        return *(PTR_size_t(PTR_CGCDesc(this))-1);
    }

    // Returns lowest series in memory.
    // Cannot be used for valuetype arrays
    PTR_CGCDescSeries GetLowestSeries ()
    {
        _ASSERTE (ptrdiff_t(GetNumSeries()) > 0);
        return PTR_CGCDescSeries(PTR_uint8_t(PTR_CGCDesc(this))
                                 - ComputeSize(GetNumSeries()));
    }

    // Returns highest series in memory.
    PTR_CGCDescSeries GetHighestSeries ()
    {
        return PTR_CGCDescSeries(PTR_size_t(PTR_CGCDesc(this))-1)-1;
    }

    // Returns number of immediate pointers this object has. It should match the number of
    // pointers enumerated by go_through_object_cl macro. The implementation shape has intentional
    // similarity with the go_through_object family of macros.
    // size is only used if you have an array of value types.
#ifndef DACCESS_COMPILE
    static size_t GetNumPointers (MethodTable* pMT, size_t ObjectSize, size_t NumComponents)
    {
        size_t NumOfPointers = 0;

        if (pMT->ContainsPointers())
        {
            CGCDesc* map = GetCGCDescFromMT(pMT);
            CGCDescSeries* cur = map->GetHighestSeries();
            ptrdiff_t cnt = (ptrdiff_t)map->GetNumSeries();

            if (cnt >= 0)
            {
                CGCDescSeries* last = map->GetLowestSeries();
                do
                {
                    NumOfPointers += (cur->GetSeriesSize() + ObjectSize) / sizeof(JSlot);
                    cur--;
                }
                while (cur >= last);
            }
            else
            {
                /* Handle the repeating case - array of valuetypes */
                for (ptrdiff_t __i = 0; __i > cnt; __i--)
                {
                    NumOfPointers += cur->val_serie[__i].nptrs;
                }

                NumOfPointers *= NumComponents;
            }
        }

#ifndef FEATURE_NATIVEAOT
        if (pMT->Collectible())
        {
            NumOfPointers += 1;
        }
#endif

        return NumOfPointers;
    }
#endif

    // Size of the entire slot map.
    size_t GetSize ()
    {
        ptrdiff_t numSeries = (ptrdiff_t) GetNumSeries();
        if (numSeries < 0)
        {
            return ComputeSizeRepeating(-numSeries);
        }
        else
        {
            return ComputeSize(numSeries);
        }
    }

    uint8_t *GetStartOfGCData()
    {
        return ((uint8_t *)this) - GetSize();
    }

private:

    BOOL IsValueClassSeries()
    {
        return ((ptrdiff_t) GetNumSeries()) < 0;
    }

};

#define MAX_SIZE_FOR_VALUECLASS_IN_ARRAY 0xffff
#define MAX_PTRS_FOR_VALUECLASSS_IN_ARRAY 0xffff

#define OBJHEADER_SIZE      (sizeof(DWORD) /* m_alignpad */ + sizeof(DWORD) /* m_SyncBlockValue */)
#define OBJECT_BASESIZE     (OBJHEADER_SIZE + OBJECT_SIZE)
#define TARGET_POINTER_SIZE 8   // equal to sizeof(void*) and the managed pointer size in bytes for this target
#define OBJECT_SIZE         TARGET_POINTER_SIZE /* m_pMethTab */

inline BOOL IS_ALIGNED( size_t val, size_t alignment )
{
    // alignment must be a power of 2 for this implementation to work (need modulo otherwise)
    _ASSERTE( 0 == (alignment & (alignment - 1)) );
    return 0 == (val & (alignment - 1));
}

extern "C" void test(MethodTable * pElemMT, MethodTable * pMT);

array.cpp

#include "shared.h"

#include <iostream>

extern "C" void test(MethodTable * pElemMT, MethodTable * pMT)
{
    CGCDescSeries* pElemSeries = CGCDesc::GetCGCDescFromMT(pElemMT)->GetHighestSeries();

    // negative series has a special meaning, indicating a different form of GCDesc
    SSIZE_T nSeries = (SSIZE_T) CGCDesc::GetCGCDescFromMT(pElemMT)->GetNumSeries();

    CGCDescSeries* pSeries = CGCDesc::GetCGCDescFromMT(pMT)->GetHighestSeries();


    for (int index = 0; index < nSeries; index ++)
    {
        size_t numPtrsInBytes = pElemSeries[-index].GetSeriesSize()
            + pElemMT->GetBaseSize();
        size_t currentOffset;
        size_t skip;
        currentOffset = pElemSeries[-index].GetSeriesOffset()+numPtrsInBytes;
        if (index != nSeries-1)
        {
            skip = pElemSeries[-(index+1)].GetSeriesOffset()-currentOffset;
        }
        else if (index == 0)
        {
            skip = pElemMT->GetNumInstanceFieldBytes() - numPtrsInBytes;
        }
        else
        {
            skip = pElemSeries[0].GetSeriesOffset() + pElemMT->GetBaseSize()
                    - OBJECT_BASESIZE - currentOffset;

        std::cout << "pElemSeries[0]: " << (void*)&pElemSeries[0] << std::endl;
        std::cout << "pElemSeries[-index]: " << (void*)&pElemSeries[-index] << std::endl;
        std::cout << "index: " << index << std::endl;
        }

        // _ASSERTE(!"Module::CreateArrayMethodTable() - unaligned GC info" || IS_ALIGNED(skip, TARGET_POINTER_SIZE));

        unsigned short NumPtrs = (unsigned short) (numPtrsInBytes / TARGET_POINTER_SIZE);
        if(skip > MAX_SIZE_FOR_VALUECLASS_IN_ARRAY || numPtrsInBytes > MAX_PTRS_FOR_VALUECLASSS_IN_ARRAY) {
            // StackSString ssElemName;
            // elemTypeHnd.GetName(ssElemName);

            // elemTypeHnd.GetAssembly()->ThrowTypeLoadException(ssElemName.GetUTF8(),
            //                                                     IDS_CLASSLOAD_VALUECLASSTOOLARGE);
        }

        val_serie_item *val_item = &(pSeries->val_serie[-index]);
        std::cout << val_item << std::endl;

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

main.cpp

#include "shared.h"

#include <vector>
#include <iostream>

int main()
{
    std::vector<char> buf1(1024 * 1024);
    std::vector<char> buf2(1024 * 1024);

    MethodTable * pElemMT = (MethodTable*)&buf1[512 * 1024];
    MethodTable * pMT = (MethodTable*)&buf2[512 * 1024];

    pElemMT->m_dwFlags = pElemMT->enum_flag_ContainsPointers;
    pMT->m_dwFlags = pMT->enum_flag_ContainsPointers;

    *(PTR_size_t(PTR_CGCDesc(CGCDesc::GetCGCDescFromMT(pElemMT)))-1) = 2;

    std::cout << "The valid ranges of memory are:" << std::endl;
    std::cout << "[" << (void*)buf1.data() << ", " << (void*)(buf1.data() + buf1.size()) << ")" << std::endl;
    std::cout << "[" << (void*)buf2.data() << ", " << (void*)(buf2.data() + buf2.size()) << ")" << std::endl;

    test(pElemMT, pMT);

    test(pElemMT, pMT);
}

Compile these files with:

g++ -O2 main.cpp array.cpp -o test

The output:

The valid ranges of memory are:
[0x7fc4d3600010, 0x7fc4d3700010)
[0x7fc4d34f0010, 0x7fc4d35f0010)
0x7fc4d356fff8
pElemSeries[0]: 0x7fc4d367fff8
pElemSeries[-index]: 0x7fc4d367ffe8
index: 1
0x7fc4d356fff0
pElemSeries[0]: 0x7fc4d367fff8
pElemSeries[-index]: 0x7fc4d367ffd8
index: 1
0x7fc4d356ffe8
pElemSeries[0]: 0x7fc4d367fff8
pElemSeries[-index]: 0x7fc4d367ffc8
index: 1
0x7fc4d356ffe0
pElemSeries[0]: 0x7fc4d367fff8
pElemSeries[-index]: 0x7fc4d367ffb8
index: 1
0x7fc4d356ffd8
pElemSeries[0]: 0x7fc4d367fff8
pElemSeries[-index]: 0x7fc4d367ffa8
index: 1
[running the same loop for a while until it segfaults]

The expected output when compiling without -O2:

The valid ranges of memory are:
[0x7fc6a8680010, 0x7fc6a8780010)
[0x7fc6a8570010, 0x7fc6a8670010)
0x7fc6a85efff8
pElemSeries[0]: 0x7fc6a86ffff8
pElemSeries[-index]: 0x7fc6a86fffe8
index: 1
0x7fc6a85efff0
0x7fc6a85efff8
pElemSeries[0]: 0x7fc6a86ffff8
pElemSeries[-index]: 0x7fc6a86fffe8
index: 1
0x7fc6a85efff0
[program exits normally at this point]

[jkotas]

Thank you!

cc @dotnet/gc