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

[Erik Österlund]

Hi Martin,

On 2019-01-15 15:01, Doerr, Martin wrote:
> 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.

It's not a POD because the standard says it is not. It is explicit about 
how you can't have private or protected member variables. Its binary 
form should indeed look like a C structure, but it's not a POD. That 
implies for example that it will be sent through the stack, not by 
register in the calling convention, unlike a POD which is sent by 
register. So despite the rules for what is and what is not a POD 
seemingly making no sense, they can't just change it to make sense 
without breaking the ABI. So I guess we are kind of stuck with it. C++11 
and later has improved the situation in this area, but we don't have it.

Unfortunately, the life cycle for non-PODs is coupled with calling 
constructors and destructors (unless trivially destructible, then you 
don't have to), while the life cycle of PODs is not; it's more like a C 
struct. That means that by making that member private, it is not a POD 
according to the definition, and suddenly you are not allowed to just 
memset it and gotta explicitly construct the object to be compliant with 
the standard.

> I believe C++ does not require NULL to be 0 before C++11.

It has to be a constant rvalue expression that evaluates to zero in C++03.

> So memset to 0 may yield undefined behavior, but we rely on this at so many places ...

Well, we are swimming in a vast ocean of undefined behaviour. It's hard 
not to, when the standard has made it as hard and unintuitive as 
possible to be compliant, and no sensible tools enforcing it.

Thanks,
/Erik

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