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

[Doerr, Martin]

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