RFR: 6294277 java -Xdebug crashes on SourceDebugExtension attribute larger than 64K

[Frederic Parain]

Coleen,

Thanks for the review.

The first allocation (jvmtiEnv.cpp) aims to create a
copy of the source_debug_extension to be returned
to the jvmti caller. The information for the native
memory tracking are provided in the implementation
of jvmtiMalloc() which calls allocate() which in turn
calls os::malloc() with a mtInternal tag.


In instanceKlass.cpp, the code manages the copy of
the source_debug_extension that will be kept with
the class metadata. The allocation is done line 1966:
1966     char* sde = NEW_C_HEAP_ARRAY(char, (length + 1), mtClass);

And the desallocation is done line 1949
1949   if (_source_debug_extension != NULL) FREE_C_HEAP_ARRAY(char, 
_source_debug_extension, mtClass);


In classFileParser.cpp, sde_buffer is just a local variable
used to hold the address of the source_debug_extension in
the classfile stream. sde_buffer is passed to the
set_source_debug_extension() method which creates a copy
of the attribute (see comment above about instanceKlass.cpp).
No reference to the attribute in the classfile stream is kept once
ClassFileParser::parse_classfile_source_debug_extension_attribute()
has completed its execution.

Regards,

Fred

On 07/ 6/12 03:49 PM, Coleen Phillimore wrote:
>
> Fred, This looks good. I had one question
>
> in jvmtiEnvi.cpp you allocate the source_debug_extension with:
>
> + *source_debug_extension_ptr = (char *) jvmtiMalloc(strlen(sde)+1);
>
>
> But in instanceKlass.cpp you deallocate it with:
>
> + if (_source_debug_extension != NULL) FREE_C_HEAP_ARRAY(u1,
> _source_debug_extension, mtClass);
>
>
> In classFileParser, cpp you set the field by pointing to the classfile
> stream.
>
> u1* sde_buffer = cfs->get_u1_buffer();
>
>
> I don't know if the first two are consistent wrt native memory tracking,
> but the last one seems dangerous if you unload that class. I think they
> should be allocated all the same with NEW_C_HEAP_ARRAY() so you can
> deallocate it that way in release_C_heap_structures.
>
> The rest looks good.
>
> Thanks,
> Coleen
>
> On 7/5/2012 8:22 AM, Frederic Parain wrote:
>> Greetings,
>>
>> The bug is described in details in the CR below.
>>
>> CR: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6294277
>> Webrev: http://cr.openjdk.java.net/~fparain/6294277/webrev.00/
>>
>> Tested with JPRT, sajdi and quick-jvmti.
>>
>> Thanks,
>>
>> Fred
>>

-- 
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: Frederic.Parain at Oracle.com