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

[Roman Kennke]

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