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

[Lois Foltan]

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.

Thanks,
Lois

>
>
> Thanks
> - Ioi
>
>> Thanks,
>> Florian
>>
>