RFR(S): 8216426: Usage of array placement new may lead to memory corruption

[Roman Kennke]

>> I had originally proposed to use placement new for each object:
>> (Kim's small request included, now)
>> http://cr.openjdk.java.net/~mdoerr/8216426_array_placement_new/webrev.00/
>>
>> So seems like we should rather use the webrev.00.
> 
> Ship it!

+1

Roman

> 
> /Robbin
> 
>>
>> Best regards,
>> Martin
>>
>>
>> -----Original Message-----
>> From: Roman Kennke <rkennke at redhat.com>
>> Sent: Dienstag, 15. Januar 2019 15:44
>> To: Doerr, Martin <martin.doerr at sap.com>; Erik Österlund
>> <erik.osterlund at oracle.com>; Robbin Ehn <robbin.ehn at oracle.com>;
>> hotspot-runtime-dev at openjdk.java.net; Kim Barrett
>> <kim.barrett at oracle.com>; David Holmes (david.holmes at oracle.com)
>> <david.holmes at oracle.com>
>> Subject: Re: RFR(S): 8216426: Usage of array placement new may lead to
>> memory corruption
>>
>> Hi Martin,
>>
>> The thing is, it is a problem all over the place, and Hotspot can't
>> currently be compiled with GCC 8, except when disabling warnings. People
>> are working to fix those issues, so for example:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8214376
>> https://bugs.openjdk.java.net/browse/JDK-8213745
>> https://bugs.openjdk.java.net/browse/JDK-8214272
>>
>> etc.
>>
>> Plus, besides, it's better style to not use memset.
>>
>> If there's a good C++ construct to achieve the same, why not use it?
>> E.g. placement new?
>>
>> Roman
>>
>>> Hi Roman and Erik,
>>>
>>> I don't have GCC 8 so I can't test it. But hotspot heavily uses
>>> memset (also with NEW_C_HEAP_ARRAY), so why should it be a problem here?
>>>
>>> I don't understand why Bucket is not a POD. Its binary form should
>>> look like a C structure.
>>> And even if it's not POD, why should it yield undefined behavior?
>>> We're not using default initialization.
>>>
>>> I believe C++ does not require NULL to be 0 before C++11. So memset
>>> to 0 may yield undefined behavior, but we rely on this at so many
>>> places ...
>>>
>>> Best regards,
>>> Martin
>>>
>>>
>>> -----Original Message-----
>>> From: Roman Kennke <rkennke at redhat.com>
>>> Sent: Dienstag, 15. Januar 2019 14:36
>>> To: Erik Österlund <erik.osterlund at oracle.com>; Doerr, Martin
>>> <martin.doerr at sap.com>; Robbin Ehn <robbin.ehn at oracle.com>;
>>> hotspot-runtime-dev at openjdk.java.net; Kim Barrett
>>> <kim.barrett at oracle.com>; David Holmes (david.holmes at oracle.com)
>>> <david.holmes at oracle.com>
>>> Subject: Re: RFR(S): 8216426: Usage of array placement new may lead
>>> to memory corruption
>>>
>>> Doesn't using memset mean that newer GCC (8) warns about it and thus
>>> break compilation? That was the whole point of introducing placement new
>>> operator to begin with, IIRC.
>>>
>>> Roman
>>>
>>>> Hi Martin,
>>>>
>>>> Unfortunately, according to the C++03 standard, that field needs to be
>>>> public for the class to be a POD. And if it is not a POD, we will have
>>>> undefined behaviour. Yay for C++.
>>>>
>>>> Thanks,
>>>> /Erik
>>>>
>>>> On 2019-01-15 11:51, Doerr, Martin wrote:
>>>>> Hi Robbin,
>>>>>
>>>>> that's fine. I like it simple.
>>>>>    Version which uses memset and removed constructor:
>>>>> http://cr.openjdk.java.net/~mdoerr/8216426_array_placement_new/webrev.01/
>>>>>
>>>>>
>>>>> I don't like making the field public because it can easily get used
>>>>> incorrectly because there are bits encoded into the pointer.
>>>>> So I think we should better not provide direct access to it.
>>>>>
>>>>> Best regards,
>>>>> Martin
>>>>>
>>>>>
>>>>> -----Original Message-----
>>>>> From: Robbin Ehn <robbin.ehn at oracle.com>
>>>>> Sent: Dienstag, 15. Januar 2019 10:26
>>>>> To: Erik Österlund <erik.osterlund at oracle.com>; Doerr, Martin
>>>>> <martin.doerr at sap.com>; hotspot-runtime-dev at openjdk.java.net; Kim
>>>>> Barrett <kim.barrett at oracle.com>; David Holmes
>>>>> (david.holmes at oracle.com) <david.holmes at oracle.com>
>>>>> Subject: Re: RFR(S): 8216426: Usage of array placement new may lead to
>>>>> memory corruption
>>>>>
>>>>> On 2019-01-15 10:15, Erik Österlund wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Perhaps an alternative approach is to make the Bucket a POD (make the
>>>>>> private member public, and remove the
>>>>>> constructor), and then simply memset the memory to zero.
>>>>> Sorry, we have NMT tracking via NEW_C_HEAP_ARRAY(Bucket, _size, F);.
>>>>> So yes, make it a POD and use memset.
>>>>>
>>>>> /Robbin
>>>>>
>>>>>> Thanks,
>>>>>> /Erik
>>>>>>
>>>>>> On 2019-01-15 09:03, Robbin Ehn wrote:
>>>>>>> Hi Martin,
>>>>>>>
>>>>>>> The Bucket should extend CHeapObj<F>, now that it could.
>>>>>>> Otherwise looks good, thanks.
>>>>>>>
>>>>>>> Originally Bucket did this and I used memset, but gdb was very upset
>>>>>>> by this.
>>>>>>> So I changed to array placement new, which meant I had to remove
>>>>>>> CHeapObj<F> if I remember correctly :)
>>>>>>>
>>>>>>> /Robbin
>>>>>>>
>>>>>>> On 2019-01-14 17:39, Doerr, Martin wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> thanks for looking at this issue and especially for adding
>>>>>>>> comments.
>>>>>>>> Sounds like this issue should better get fixed although there are
>>>>>>>> no known problems.
>>>>>>>>
>>>>>>>> Should we fix it by replacing the array placement new by normal
>>>>>>>> placement new for each element?
>>>>>>>> http://cr.openjdk.java.net/~mdoerr/8216426_array_placement_new/webrev.00/
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> Martin
>>>>>>>>
>>>>
>>>
>>