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

[Robbin Ehn]

On 2019-01-15 16:48, Doerr, Martin wrote:
> Hi Roman,
> 
> 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!

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