RFR(XXS) 8249087 Symbol constructor unnecessarily initializes _body[0]

[Lois Foltan]

On 7/22/2020 12:17 AM, David Holmes wrote:
> Hi Lois,
>
> On 22/07/2020 1:06 am, Lois Foltan wrote:
>> On 7/21/2020 2:24 AM, Ioi Lam wrote:
>>>
>>>
>>> On 7/20/20 11:12 PM, Florian Weimer wrote:
>>>> * Ioi Lam:
>>>>
>>>>> Hi please review this very simple fix:
>>>>>
>>>>> diff -r 19f26d72a8d0 src/hotspot/share/oops/symbol.cpp
>>>>> --- a/src/hotspot/share/oops/symbol.cpp    Mon Jul 20 14:24:19 
>>>>> 2020 -0700
>>>>> +++ b/src/hotspot/share/oops/symbol.cpp    Mon Jul 20 17:11:57 
>>>>> 2020 -0700
>>>>> @@ -51,8 +51,11 @@
>>>>>   Symbol::Symbol(const u1* name, int length, int refcount) {
>>>>>     _hash_and_refcount = pack_hash_and_refcount((short)os::random(),
>>>>> refcount);
>>>>>     _length = length;
>>>>> -  _body[0] = 0;  // in case length == 0
>>>>>     memcpy(_body, name, length);
>>>>> +  // For symbols of length 0 and 1: _body[0] (and _body[1]) are
>>>>> uninitialized and may
>>>>> +  // contain random values, which will only be read by
>>>>> Symbol::identity_hash(),
>>>>> +  // which would tolerate such randomness. These values never change
>>>>> during the lifetime
>>>>> +  // of the Symbol.
>>>>>   }
>>>> Won't this still trip memory debuggers? Symbol::identity_hash() 
>>>> implies
>>>> that the result is eventually used in a conditional operation (a hash
>>>> comparison perhaps).  If it's possible one day to run Hotspot under
>>>> valgrind, this would result in false positives.
>>>
>>> Are you saying that valgrind will modify uninitialized memory 
>>> periodically after the constructor has returned, and thus will cause 
>>> Symbol::identity_hash() to return a different value?
>>>
>>> Without my patch, _body[1] is uninitialized for Symbols whose length 
>>> is 0 or 1. We have not heard of any issues related to valgrind and 
>>> Symbol::identity_hash().
>>>
>>> In fact, looking at the code history, the setting of "_body[0] = 0" 
>>> in Symbol::Symbol was introduced only recently (Feb 2020):
>>>
>>> http://hg.openjdk.java.net/jdk/jdk/annotate/4a4d185098e2/src/hotspot/share/oops/symbol.cpp#l55 
>>>
>>>
>>> I'll check with Lois who added the code to see the reason for doing it.
>>
>> Hi Ioi,
>>
>> Reviewing this JBS issue, I have concerns over leaving both _body[0] 
>> and now even _body[1] uninitialized.  The signature processing 
>> frequently checks the first character of a Symbol via 
>> Symbol::char_at(0) to determine what type it is dealing with. Is 
>> there a danger that the uninitialized memory actually has a valid 
>> type indicator in it like an '[' character for example? The signature 
>> processing could potentially make wrong assumptions about the type it 
>> is trying to process.
>
> Aren't all the signature related symbols already guaranteed to not 
> have zero-length, or else the length is being pre-tested for zero?

Hi David,

I believe you are correct in that signature related symbols are already 
guaranteed to not have zero-length.  I tried several different jasm 
files to introduce an empty string either for a class name or for a type 
of a field reference and did not get past class file parsing without a 
ClassFormatError.

Error: LinkageError occurred while loading main class HelloEmptyString
         java.lang.ClassFormatError: Class name is empty or contains 
illegal character in descriptor in class file HelloEmptyString


However, after reading the original issue in JDK-8249087, the objection 
wasn't that there was a needless initialization of _body[0] but instead 
it was around the inconsistency of initializing _body[0] and not 
_body[1].  I think it should be the responsibility of the Symbol class 
API to ensure initialization so that as a consumer I don't have to worry 
about _body[0] or _body[1]'s validity.  So I prefer the change to 
actually add the initialization of _body[1].  Something like, "_body[0] 
= _body[1] = 0".

Thanks,
Lois

>
> Thanks,
> David
> -----
>
>> Thanks,
>> Lois
>>
>>>
>>>
>>> Thanks
>>> - Ioi
>>>
>>>> Thanks,
>>>> Florian
>>>>
>>>
>>