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

[David Holmes]

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
>>>
>>
>