RFR: Out-of-bounds access in cpu_family_description()

[Jinke Fan]

Hi David,
     This seems good to me.

Best Regards!
Fanjinke
On 2019/4/12 15:50, David Holmes wrote:
> Hi Fanjinke,
> 
> I have filed:
> 
> https://bugs.openjdk.java.net/browse/JDK-8222387
> 
> On 12/04/2019 4:51 pm, Jinke Fan wrote:
>> On 2019/4/12 14:20, David Holmes wrote:
>>> Hi,
>>>
>>> Was there a reason you had to move the existing arrays before 
>>> extending the amd one with the missing values?
>> Yes, because the addition uses sizeof to control the boundary access.
>> If the arrays's defined were behind the sizeof,it causes a compilation 
>> error: "error: invalid application of 'sizeof’ to incomplete type"
> 
> Okay I found that a little crude so I went for a slightly nicer approach:
> 
> http://cr.openjdk.java.net/~dholmes/8222387/webrev/
> 
> Hope you don't mind.
> 
> Thanks,
> David
> 
>> Best Regards!
>> Fanjinke
>>>
>>> Thanks,
>>> David
>>>
>>> On 12/04/2019 3:47 pm, Jinke Fan wrote:
>>>> Hi David,
>>>>      In VM_Version_Ext::cpu_family_description has out-of-bounds
>>>> access on AMD 17h (EPYC) processor.
>>>>
>>>> const char* VM_Version_Ext::cpu_family_description(void) {
>>>>
>>>> On AMD 17h (EPYC) processor extended_cpu_family() will return 23,
>>>> but array _family_id_amd only has 17 members.
>>>>
>>>>    int cpu_family_id = extended_cpu_family();
>>>>    if (is_amd()) {
>>>>      return _family_id_amd[cpu_family_id];
>>>>    }
>>>> ...
>>>> }
>>>>
>>>> Result of testcase TestCPUInformation.java on AMD Zen:
>>>> ----------System.out:(15/1615)----------
>>>> ...
>>>> Family: 386 (0x17), Model: <unknown> (0x1), Stepping: 0x1
>>>> Ext. family: 0x8, Ext. model: 0x0, Type: 0x0, Signature: 0x00800f11
>>>> ...
>>>> }
>>>>
>>>> The “386” string is incorrectly and comes from Illegal access.
>>>>
>>>> The patch is based on the original repository:
>>>> hg.openjdk.java.net/jdk/jdk
>>>>
>>>> changeset:   54520:f48312257bc6
>>>> tag:         tip
>>>> user:        vromero
>>>> date:        Thu Apr 11 22:56:11 2019 -0400
>>>> summary:     8222151: refactoring: enhancements to 
>>>> java.lang.Class::methodToString and java.lang.Class::getTypeName
>>>>
>>>> *Patch
>>>> The out of hg diff -g:
>>>> diff --git a/src/hotspot/cpu/x86/vm_version_ext_x86.cpp 
>>>> b/src/hotspot/cpu/x86/vm_version_ext_x86.cpp
>>>> --- a/src/hotspot/cpu/x86/vm_version_ext_x86.cpp
>>>> +++ b/src/hotspot/cpu/x86/vm_version_ext_x86.cpp
>>>> @@ -262,6 +262,52 @@
>>>>   int VM_Version_Ext::_no_of_cores = 0;
>>>>   int VM_Version_Ext::_no_of_packages = 0;
>>>>
>>>> +const char* const VM_Version_Ext::_family_id_intel[] = {
>>>> +  "8086/8088",
>>>> +  "",
>>>> +  "286",
>>>> +  "386",
>>>> +  "486",
>>>> +  "Pentium",
>>>> +  "Pentium Pro",   //or Pentium-M/Woodcrest depeding on model
>>>> +  "",
>>>> +  "",
>>>> +  "",
>>>> +  "",
>>>> +  "",
>>>> +  "",
>>>> +  "",
>>>> +  "",
>>>> +  "Pentium 4"
>>>> +};
>>>> +
>>>> +const char* const VM_Version_Ext::_family_id_amd[] = {
>>>> +  "",
>>>> +  "",
>>>> +  "",
>>>> +  "",
>>>> +  "5x86",
>>>> +  "K5/K6",
>>>> +  "Athlon/AthlonXP",
>>>> +  "",
>>>> +  "",
>>>> +  "",
>>>> +  "",
>>>> +  "",
>>>> +  "",
>>>> +  "",
>>>> +  "",
>>>> +  "Opteron/Athlon64",
>>>> +  "Opteron QC/Phenom",  // Barcelona et.al.
>>>> +  "",
>>>> +  "",
>>>> +  "",
>>>> +  "",
>>>> +  "",
>>>> +  "",
>>>> +  "Zen"
>>>> +};
>>>> +
>>>>   void VM_Version_Ext::initialize(void) {
>>>>     ResourceMark rm;
>>>>
>>>> @@ -401,15 +447,19 @@
>>>>   }
>>>>
>>>>   const char* VM_Version_Ext::cpu_family_description(void) {
>>>> -  int cpu_family_id = extended_cpu_family();
>>>> +  uint32_t cpu_family_id = extended_cpu_family();
>>>>     if (is_amd()) {
>>>> -    return _family_id_amd[cpu_family_id];
>>>> +    if (cpu_family_id < 
>>>> sizeof(_family_id_amd)/sizeof(_family_id_amd[0])) {
>>>> +      return _family_id_amd[cpu_family_id];
>>>> +    }
>>>>     }
>>>>     if (is_intel()) {
>>>>       if (cpu_family_id == CPU_FAMILY_PENTIUMPRO) {
>>>>         return cpu_model_description();
>>>>       }
>>>> -    return _family_id_intel[cpu_family_id];
>>>> +    if (cpu_family_id < 
>>>> sizeof(_family_id_intel)/sizeof(_family_id_intel[0])) {
>>>> +      return _family_id_intel[cpu_family_id];
>>>> +    }
>>>>     }
>>>>     if (is_hygon()) {
>>>>       return "Dhyana";
>>>> @@ -705,44 +755,6 @@
>>>>     return _max_qualified_cpu_frequency;
>>>>   }
>>>>
>>>> -const char* const VM_Version_Ext::_family_id_intel[] = {
>>>> -  "8086/8088",
>>>> -  "",
>>>> -  "286",
>>>> -  "386",
>>>> -  "486",
>>>> -  "Pentium",
>>>> -  "Pentium Pro",   //or Pentium-M/Woodcrest depeding on model
>>>> -  "",
>>>> -  "",
>>>> -  "",
>>>> -  "",
>>>> -  "",
>>>> -  "",
>>>> -  "",
>>>> -  "",
>>>> -  "Pentium 4"
>>>> -};
>>>> -
>>>> -const char* const VM_Version_Ext::_family_id_amd[] = {
>>>> -  "",
>>>> -  "",
>>>> -  "",
>>>> -  "",
>>>> -  "5x86",
>>>> -  "K5/K6",
>>>> -  "Athlon/AthlonXP",
>>>> -  "",
>>>> -  "",
>>>> -  "",
>>>> -  "",
>>>> -  "",
>>>> -  "",
>>>> -  "",
>>>> -  "",
>>>> -  "Opteron/Athlon64",
>>>> -  "Opteron QC/Phenom"  // Barcelona et.al.
>>>> -};
>>>>   // Partially from Intel 64 and IA-32 Architecture Software 
>>>> Developer's Manual,
>>>>   // September 2013, Vol 3C Table 35-1
>>>>   const char* const VM_Version_Ext::_model_id_pentium_pro[] = {
>>>>
>>>> *Test:
>>>> After patched,result of testcase TestCPUInformation.java on AMD Zen:
>>>> ----------System.out:(15/1615)----------
>>>> Event: jdk.CPUInformation {
>>>>    ...
>>>> Family: Zen (0x17), Model: <unknown> (0x1), Stepping: 0x1
>>>> Ext. family: 0x8, Ext. model: 0x0, Type: 0x0, Signature: 0x00800f11
>>>> Features: ebx: 0x4f400800, ecx: 0x7ed8320b, edx: 0x178bfbff
>>>>    ...
>>>> }
>>>>
>>>> Is there anything incorrectly?
>>>> Please let me know your comments.
>>>>
>>>> Best Regards！
>>>> Fanjinke
>>>>
>>>
>>
>