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

Tue Jan 15 16:41:45 UTC 2019

Hi,

thanks everyone for reviewing.

I also had the idea to introduce something like NEW_CLEARED_C_HEAP_ARRAY.

Best regards,
Martin

-----Original Message-----
From: Erik Österlund <erik.osterlund at oracle.com> 
Sent: Dienstag, 15. Januar 2019 17:32
To: Doerr, Martin <martin.doerr at sap.com>; Roman Kennke <rkennke at redhat.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,

I'm okay with this solution. Maybe we rethink things after C++14.

I wonder if NEW_C_HEAP_ARRAY or a similar macro should both allocate and 
initialize, so that users of the API don't have to trip over the usual 
(not quite obvious) UB land mines every time an array is allocated, when 
the solution is typically the same.

Thanks,
/Erik

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