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

[David Holmes]

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?

Thanks,
David
-----

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