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

[Erik Österlund]

Hi Roman,

Using memset on non-PODs is indeed undefined behaviour. But for PODs, it 
is not. So I presume GCC warns about incorrect usages involving non-PODs.

/Erik

On 2019-01-15 14:36, Roman Kennke wrote:
> 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
>>>>>>