Quantcast

RFR: 8170812: Metaspace corruption caused by incorrect memory size for MethodCounters

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR: 8170812: Metaspace corruption caused by incorrect memory size for MethodCounters

Andrew Haley
If sizeof (MethodCounters) is not a multiple of wordSize, memory
allocator metadata is corrupted, causing the VM to become unstable and
eventually crash.

The fix is very simple:

diff -r 85b6ca9458ed src/share/vm/oops/methodCounters.hpp
--- a/src/share/vm/oops/methodCounters.hpp      Wed Mar 29 15:44:34 2017 +0000
+++ b/src/share/vm/oops/methodCounters.hpp      Wed Apr 05 15:42:18 2017 +0100
@@ -116,7 +116,7 @@

   AOT_ONLY(Method* method() const { return _method; })

-  static int size() { return sizeof(MethodCounters) / wordSize; }
+  static int size() { return align_size_up(sizeof(MethodCounters), wordSize) / wordSize; }

   bool is_klass() const { return false; }

This is very low risk because if the size is already a multiple of
wordSize, this patch will have no effect.  If the size is not a
multiple of wordSize, this patch will prevent an inevitable crash.

I've applied for a JDK9 fix request.  I'll need a sponsor.

Andrew.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8170812: Metaspace corruption caused by incorrect memory size for MethodCounters

Kim Barrett
> On Apr 5, 2017, at 11:13 AM, Andrew Haley <[hidden email]> wrote:
>
> If sizeof (MethodCounters) is not a multiple of wordSize, memory
> allocator metadata is corrupted, causing the VM to become unstable and
> eventually crash.
>
> The fix is very simple:
>
> diff -r 85b6ca9458ed src/share/vm/oops/methodCounters.hpp
> --- a/src/share/vm/oops/methodCounters.hpp      Wed Mar 29 15:44:34 2017 +0000
> +++ b/src/share/vm/oops/methodCounters.hpp      Wed Apr 05 15:42:18 2017 +0100
> @@ -116,7 +116,7 @@
>
>   AOT_ONLY(Method* method() const { return _method; })
>
> -  static int size() { return sizeof(MethodCounters) / wordSize; }
> +  static int size() { return align_size_up(sizeof(MethodCounters), wordSize) / wordSize; }
>
>   bool is_klass() const { return false; }
>
> This is very low risk because if the size is already a multiple of
> wordSize, this patch will have no effect.  If the size is not a
> multiple of wordSize, this patch will prevent an inevitable crash.
>
> I've applied for a JDK9 fix request.  I'll need a sponsor.
>
> Andrew.

 It looks like it might require a fairly specific set of build options, but yikes!

Change looks good.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8170812: Metaspace corruption caused by incorrect memory size for MethodCounters

Andrew Haley
On 06/04/17 02:31, Kim Barrett wrote:

>> On Apr 5, 2017, at 11:13 AM, Andrew Haley <[hidden email]> wrote:
>>
>> If sizeof (MethodCounters) is not a multiple of wordSize, memory
>> allocator metadata is corrupted, causing the VM to become unstable and
>> eventually crash.
>>
>> I've applied for a JDK9 fix request.  I'll need a sponsor.
>>
>> Andrew.
>
>  It looks like it might require a fairly specific set of build options, but yikes!
>
> Change looks good.

Yikes indeed.  :-)

Webrev at http://cr.openjdk.java.net/~aph/8170812/.

Thanks,

Andrew.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8170812: Metaspace corruption caused by incorrect memory size for MethodCounters

Thomas Stüfe-2
In reply to this post by Andrew Haley
:(

Don't the other childs of MetaspaceObj
(e.g. Annotations, ConstMethod, MethodCounters) have the same problem?

..Thomas

On Wed, Apr 5, 2017 at 5:13 PM, Andrew Haley <[hidden email]> wrote:

> If sizeof (MethodCounters) is not a multiple of wordSize, memory
> allocator metadata is corrupted, causing the VM to become unstable and
> eventually crash.
>
> The fix is very simple:
>
> diff -r 85b6ca9458ed src/share/vm/oops/methodCounters.hpp
> --- a/src/share/vm/oops/methodCounters.hpp      Wed Mar 29 15:44:34 2017
> +0000
> +++ b/src/share/vm/oops/methodCounters.hpp      Wed Apr 05 15:42:18 2017
> +0100
> @@ -116,7 +116,7 @@
>
>    AOT_ONLY(Method* method() const { return _method; })
>
> -  static int size() { return sizeof(MethodCounters) / wordSize; }
> +  static int size() { return align_size_up(sizeof(MethodCounters),
> wordSize) / wordSize; }
>
>    bool is_klass() const { return false; }
>
> This is very low risk because if the size is already a multiple of
> wordSize, this patch will have no effect.  If the size is not a
> multiple of wordSize, this patch will prevent an inevitable crash.
>
> I've applied for a JDK9 fix request.  I'll need a sponsor.
>
> Andrew.
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8170812: Metaspace corruption caused by incorrect memory size for MethodCounters

Andrew Haley
On 06/04/17 10:05, Thomas Stüfe wrote:
>
> Don't the other childs of MetaspaceObj (e.g. Annotations, ConstMethod, MethodCounters) have the same problem?

Possibly.  If anyone thinks I should fix them all, I will.

Andrew.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8170812: Metaspace corruption caused by incorrect memory size for MethodCounters

Thomas Stüfe-2
On Thu, Apr 6, 2017 at 11:09 AM, Andrew Haley <[hidden email]> wrote:

> On 06/04/17 10:05, Thomas Stüfe wrote:
> >
> > Don't the other childs of MetaspaceObj (e.g. Annotations, ConstMethod,
> MethodCounters) have the same problem?
>
> Possibly.  If anyone thinks I should fix them all, I will.
>
>
I think it would make sense. From what I can see, the following methods are
affected:

ConstantPool::header_size()
ConstMethod::header_size()
ConstantPoolCache::header_size()
Method::header_size()
MethodCounters::size()

Worst thing the alignment is a noop because the class sizes are already
multiples of MetaWord.

Curiously, there is a function named "align_metadata_size" which gets
called in many cases but does not really do much:

inline intptr_t align_metadata_size(intptr_t size) {
  return align_size_up(size, 1);
}

..Thomas



> Andrew.
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8170812: Metaspace corruption caused by incorrect memory size for MethodCounters

Andrew Haley
On 06/04/17 11:35, Thomas Stüfe wrote:
>
> I think it would make sense. From what I can see, the following methods are affected:
>
> ConstantPool::header_size()

Not this one, because the declaration of ConstantPoolCache is

  int             _length;
  ConstantPool*   _constant_pool;          // the corresponding constant pool

The size of this declaration must be a multiple of wordSize
because the fields must be allocated in order, they must be
aligned, and _constant_pool comes last.

It's very confusing code, but not actually a bug, IMO.

Andrew.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8170812: Metaspace corruption caused by incorrect memory size for MethodCounters

coleen.phillimore
In reply to this post by Andrew Haley

I'm confused.  Metaspace is allocated in granularity of 3 pointer sized
words.

Coleen

On 4/5/17 11:13 AM, Andrew Haley wrote:

> If sizeof (MethodCounters) is not a multiple of wordSize, memory
> allocator metadata is corrupted, causing the VM to become unstable and
> eventually crash.
>
> The fix is very simple:
>
> diff -r 85b6ca9458ed src/share/vm/oops/methodCounters.hpp
> --- a/src/share/vm/oops/methodCounters.hpp      Wed Mar 29 15:44:34 2017 +0000
> +++ b/src/share/vm/oops/methodCounters.hpp      Wed Apr 05 15:42:18 2017 +0100
> @@ -116,7 +116,7 @@
>
>     AOT_ONLY(Method* method() const { return _method; })
>
> -  static int size() { return sizeof(MethodCounters) / wordSize; }
> +  static int size() { return align_size_up(sizeof(MethodCounters), wordSize) / wordSize; }
>
>     bool is_klass() const { return false; }
>
> This is very low risk because if the size is already a multiple of
> wordSize, this patch will have no effect.  If the size is not a
> multiple of wordSize, this patch will prevent an inevitable crash.
>
> I've applied for a JDK9 fix request.  I'll need a sponsor.
>
> Andrew.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8170812: Metaspace corruption caused by incorrect memory size for MethodCounters

Ioi Lam
On 4/6/17 8:33 PM, [hidden email] wrote:
>
> I'm confused.  Metaspace is allocated in granularity of 3 pointer
> sized words.
>
But sizeof(MethodCounters) is pretty big (72 bytes on linux/x86/debug).
If it becomes, say 76 bytes, due to some build config options,

           sizeof(MethodCounters) / wordSize

will cause the last 4 bytes to be chopped.

- Ioi

> Coleen
>
> On 4/5/17 11:13 AM, Andrew Haley wrote:
>> If sizeof (MethodCounters) is not a multiple of wordSize, memory
>> allocator metadata is corrupted, causing the VM to become unstable and
>> eventually crash.
>>
>> The fix is very simple:
>>
>> diff -r 85b6ca9458ed src/share/vm/oops/methodCounters.hpp
>> --- a/src/share/vm/oops/methodCounters.hpp      Wed Mar 29 15:44:34
>> 2017 +0000
>> +++ b/src/share/vm/oops/methodCounters.hpp      Wed Apr 05 15:42:18
>> 2017 +0100
>> @@ -116,7 +116,7 @@
>>
>>     AOT_ONLY(Method* method() const { return _method; })
>>
>> -  static int size() { return sizeof(MethodCounters) / wordSize; }
>> +  static int size() { return align_size_up(sizeof(MethodCounters),
>> wordSize) / wordSize; }
>>
>>     bool is_klass() const { return false; }
>>
>> This is very low risk because if the size is already a multiple of
>> wordSize, this patch will have no effect.  If the size is not a
>> multiple of wordSize, this patch will prevent an inevitable crash.
>>
>> I've applied for a JDK9 fix request.  I'll need a sponsor.
>>
>> Andrew.
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8170812: Metaspace corruption caused by incorrect memory size for MethodCounters

coleen.phillimore


On 4/6/17 8:50 AM, Ioi Lam wrote:

> On 4/6/17 8:33 PM, [hidden email] wrote:
>>
>> I'm confused.  Metaspace is allocated in granularity of 3 pointer
>> sized words.
>>
> But sizeof(MethodCounters) is pretty big (72 bytes on
> linux/x86/debug). If it becomes, say 76 bytes, due to some build
> config options,
>
>           sizeof(MethodCounters) / wordSize
>
> will cause the last 4 bytes to be chopped.

I see.  It isn't that it's too small.   I think all of the allocation
functions might have this, except the last thing in these classes is
typically a pointer so they aren't unaligned.

I will sponsor this, Andrew.  Can you check the other ones?

Thanks!
Coleen

>
> - Ioi
>> Coleen
>>
>> On 4/5/17 11:13 AM, Andrew Haley wrote:
>>> If sizeof (MethodCounters) is not a multiple of wordSize, memory
>>> allocator metadata is corrupted, causing the VM to become unstable and
>>> eventually crash.
>>>
>>> The fix is very simple:
>>>
>>> diff -r 85b6ca9458ed src/share/vm/oops/methodCounters.hpp
>>> --- a/src/share/vm/oops/methodCounters.hpp      Wed Mar 29 15:44:34
>>> 2017 +0000
>>> +++ b/src/share/vm/oops/methodCounters.hpp      Wed Apr 05 15:42:18
>>> 2017 +0100
>>> @@ -116,7 +116,7 @@
>>>
>>>     AOT_ONLY(Method* method() const { return _method; })
>>>
>>> -  static int size() { return sizeof(MethodCounters) / wordSize; }
>>> +  static int size() { return align_size_up(sizeof(MethodCounters),
>>> wordSize) / wordSize; }
>>>
>>>     bool is_klass() const { return false; }
>>>
>>> This is very low risk because if the size is already a multiple of
>>> wordSize, this patch will have no effect.  If the size is not a
>>> multiple of wordSize, this patch will prevent an inevitable crash.
>>>
>>> I've applied for a JDK9 fix request.  I'll need a sponsor.
>>>
>>> Andrew.
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8170812: Metaspace corruption caused by incorrect memory size for MethodCounters

Andrew Haley
On 06/04/17 13:56, [hidden email] wrote:
> I see.  It isn't that it's too small.   I think all of the allocation
> functions might have this, except the last thing in these classes is
> typically a pointer so they aren't unaligned.
>
> I will sponsor this, Andrew.  Can you check the other ones?

Yes.  Do you want me to leave the ones which always end with a
pointer for JDK10, or fix the lot now?

Andrew.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8170812: Metaspace corruption caused by incorrect memory size for MethodCounters

Thomas Stüfe-2
In reply to this post by Andrew Haley
Hi Andrew,

On Thu, Apr 6, 2017 at 12:57 PM, Andrew Haley <[hidden email]> wrote:

> On 06/04/17 11:35, Thomas Stüfe wrote:
> >
> > I think it would make sense. From what I can see, the following methods
> are affected:
> >
> > ConstantPool::header_size()
>
> Not this one, because the declaration of ConstantPoolCache is
>
>   int             _length;
>   ConstantPool*   _constant_pool;          // the corresponding constant
> pool
>
> The size of this declaration must be a multiple of wordSize
> because the fields must be allocated in order, they must be
> aligned, and _constant_pool comes last.
>
> It's very confusing code, but not actually a bug, IMO.
>
>
I am confused:

ConstantPool::header_size() is used by ConstantPool::size(len), which in
turn is used in ConstantPool::allocate() to allocate space for a
ConstantPool. header_size() may be too small, and so the space allocated
for the whole ConstantPool plus data may be too small, no?

..Thomas



> Andrew.
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8170812: Metaspace corruption caused by incorrect memory size for MethodCounters

coleen.phillimore
Deciding...   I say fix the lot now.  I think it's low risk to do so and will save filing another bug etc.
Thanks!
Coleen

> On Apr 6, 2017, at 9:02 AM, Thomas Stüfe <[hidden email]> wrote:
>
> Hi Andrew,
>
>> On Thu, Apr 6, 2017 at 12:57 PM, Andrew Haley <[hidden email]> wrote:
>>
>>> On 06/04/17 11:35, Thomas Stüfe wrote:
>>>
>>> I think it would make sense. From what I can see, the following methods
>> are affected:
>>>
>>> ConstantPool::header_size()
>>
>> Not this one, because the declaration of ConstantPoolCache is
>>
>>  int             _length;
>>  ConstantPool*   _constant_pool;          // the corresponding constant
>> pool
>>
>> The size of this declaration must be a multiple of wordSize
>> because the fields must be allocated in order, they must be
>> aligned, and _constant_pool comes last.
>>
>> It's very confusing code, but not actually a bug, IMO.
>>
>>
> I am confused:
>
> ConstantPool::header_size() is used by ConstantPool::size(len), which in
> turn is used in ConstantPool::allocate() to allocate space for a
> ConstantPool. header_size() may be too small, and so the space allocated
> for the whole ConstantPool plus data may be too small, no?
>
> ..Thomas
>
>
>
>> Andrew.
>>
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8170812: Metaspace corruption caused by incorrect memory size for MethodCounters

Ioi Lam
I think we should also fix align_metadata_size() and use that for the
size() function of all MetaspaceObj classes.

Thanks
- Ioi

On 4/6/17 9:20 PM, Coleen Phillimore wrote:

> Deciding...   I say fix the lot now.  I think it's low risk to do so and will save filing another bug etc.
> Thanks!
> Coleen
>
>> On Apr 6, 2017, at 9:02 AM, Thomas Stüfe <[hidden email]> wrote:
>>
>> Hi Andrew,
>>
>>> On Thu, Apr 6, 2017 at 12:57 PM, Andrew Haley <[hidden email]> wrote:
>>>
>>>> On 06/04/17 11:35, Thomas Stüfe wrote:
>>>>
>>>> I think it would make sense. From what I can see, the following methods
>>> are affected:
>>>> ConstantPool::header_size()
>>> Not this one, because the declaration of ConstantPoolCache is
>>>
>>>   int             _length;
>>>   ConstantPool*   _constant_pool;          // the corresponding constant
>>> pool
>>>
>>> The size of this declaration must be a multiple of wordSize
>>> because the fields must be allocated in order, they must be
>>> aligned, and _constant_pool comes last.
>>>
>>> It's very confusing code, but not actually a bug, IMO.
>>>
>>>
>> I am confused:
>>
>> ConstantPool::header_size() is used by ConstantPool::size(len), which in
>> turn is used in ConstantPool::allocate() to allocate space for a
>> ConstantPool. header_size() may be too small, and so the space allocated
>> for the whole ConstantPool plus data may be too small, no?
>>
>> ..Thomas
>>
>>
>>
>>> Andrew.
>>>
>>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8170812: Metaspace corruption caused by incorrect memory size for MethodCounters

Andrew Haley
On 06/04/17 14:29, Ioi Lam wrote:
> I think we should also fix align_metadata_size() and use that for the
> size() function of all MetaspaceObj classes.

IMO: in this phase let's do the minimum correct thing, which is
calling align_size_up() in all of the size() methods.

Andrew.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8170812: Metaspace corruption caused by incorrect memory size for MethodCounters

Mikael Vidstedt-3
In reply to this post by Andrew Haley

Worth adding a few helpful asserts somewhere? I’m thinking something like:

assert(MethodCounters::size() >= sizeof(MethodCounters));

Cheers,
Mikael

> On Apr 5, 2017, at 8:13 AM, Andrew Haley <[hidden email]> wrote:
>
> If sizeof (MethodCounters) is not a multiple of wordSize, memory
> allocator metadata is corrupted, causing the VM to become unstable and
> eventually crash.
>
> The fix is very simple:
>
> diff -r 85b6ca9458ed src/share/vm/oops/methodCounters.hpp
> --- a/src/share/vm/oops/methodCounters.hpp      Wed Mar 29 15:44:34 2017 +0000
> +++ b/src/share/vm/oops/methodCounters.hpp      Wed Apr 05 15:42:18 2017 +0100
> @@ -116,7 +116,7 @@
>
>   AOT_ONLY(Method* method() const { return _method; })
>
> -  static int size() { return sizeof(MethodCounters) / wordSize; }
> +  static int size() { return align_size_up(sizeof(MethodCounters), wordSize) / wordSize; }
>
>   bool is_klass() const { return false; }
>
> This is very low risk because if the size is already a multiple of
> wordSize, this patch will have no effect.  If the size is not a
> multiple of wordSize, this patch will prevent an inevitable crash.
>
> I've applied for a JDK9 fix request.  I'll need a sponsor.
>
> Andrew.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8170812: Metaspace corruption caused by incorrect memory size for MethodCounters

Andrew Haley
In reply to this post by coleen.phillimore
On 06/04/17 14:20, Coleen Phillimore wrote:
> Deciding...   I say fix the lot now.  I think it's low risk to do so and will save filing another bug etc.
> Thanks!


http://cr.openjdk.java.net/~aph/8170812-2/

Andrew.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8170812: Metaspace corruption caused by incorrect memory size for MethodCounters

Kim Barrett
> On Apr 7, 2017, at 8:48 AM, Andrew Haley <[hidden email]> wrote:
>
> On 06/04/17 14:20, Coleen Phillimore wrote:
>> Deciding...   I say fix the lot now.  I think it's low risk to do so and will save filing another bug etc.
>> Thanks!
>
>
> http://cr.openjdk.java.net/~aph/8170812-2/
>
> Andrew.

Looks good.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8170812: Metaspace corruption caused by incorrect memory size for MethodCounters

Andrew Haley
On 07/04/17 15:10, Kim Barrett wrote:

>> On Apr 7, 2017, at 8:48 AM, Andrew Haley <[hidden email]> wrote:
>>
>> On 06/04/17 14:20, Coleen Phillimore wrote:
>>> Deciding...   I say fix the lot now.  I think it's low risk to do so and will save filing another bug etc.
>>> Thanks!
>>
>>
>> http://cr.openjdk.java.net/~aph/8170812-2/
>>
>> Andrew.
>
> Looks good.

OK.  Will you please push this?

Thanks,

Andrew.


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8170812: Metaspace corruption caused by incorrect memory size for MethodCounters

coleen.phillimore


On 4/7/17 10:20 AM, Andrew Haley wrote:

> On 07/04/17 15:10, Kim Barrett wrote:
>>> On Apr 7, 2017, at 8:48 AM, Andrew Haley <[hidden email]> wrote:
>>>
>>> On 06/04/17 14:20, Coleen Phillimore wrote:
>>>> Deciding...   I say fix the lot now.  I think it's low risk to do so and will save filing another bug etc.
>>>> Thanks!
>>>
>>> http://cr.openjdk.java.net/~aph/8170812-2/
>>>
>>> Andrew.
>> Looks good.
> OK.  Will you please push this?

yes, this looks good to me also.  I'm in the process of pushing it.

thanks for finding this and the fix.
Coleen
>
> Thanks,
>
> Andrew.
>
>

Loading...