RFR (S) 8178870: instrumentation.retransformClasses cause coredump

[coleen.phillimore at oracle.com]

Thanks Serguei for reviewing and discussion about this bug.

On 10/5/17 4:19 PM, serguei.spitsyn at oracle.com wrote:
> Hi Coleen,
>
> The fix look good.
> - if (the_class->get_cached_class_file_bytes() == 0) {
> + if (the_class->get_cached_class_file() == 0) {
> . . .
> - else if (scratch_class->get_cached_class_file_bytes() !=
> - the_class->get_cached_class_file_bytes()) {
> + else if (scratch_class->get_cached_class_file() !=
> + the_class->get_cached_class_file()) {
> Thank you for additionally fixing the part above!
> I was surprised that the original code 
> usedget_cached_class_file_bytes() instead ofget_cached_class_file() .
>
> Nice test!

Thank you!
>
> A couple of minor comments.
>
> http://cr.openjdk.java.net/~coleenp/8178870.01/webrev/test/hotspot/jtreg/runtime/RedefineTests/RedefineDoubleDelete.java.html
>    47                 "   int faa() { System.out.println(\"baa\"); return 2; }" +
>    A typo: baa => faa
>

I want the redefined guy to say baa.
>
> http://cr.openjdk.java.net/~coleenp/8178870.01/webrev/test/hotspot/jtreg/runtime/RedefineTests/libRedefineDoubleDelete.c.html
>    78     int pos = 0;
>    79     int found = 0;
>    80     int i;
>    81     for (i = 0; i < newClassDataLen; i++) {
>    82         newClassData[i] = class_data[i];
>    83         // Rewrite oo in class to aa
>    84         if (class_data[i] == 'o') {
>    85             if (i == pos + 1) {
>    86                newClassData[i-1] = 'a';
>    87                newClassData[i] = 'a';
>    88             } else {
>    89                pos = i;
>    90             }
>    91         }
>    92     }
>   The local 'found' is not used.
>   The fragment can be simplified a little bit:
>         for (i = 0; i < newClassDataLen; i++) {
>             newClassData[i] = class_data[i];
>             // Rewrite oo in class to aa
>             if (i > 0 && class_data[i] == 'o' &&  class_data[i-1] == 'o') {
>                 newClassData[i] = newClassData[i-1] = 'a';
>             }
>         }
>
Yes, this is better.   I've made this change:

     int i;
     for (i = 0; i < newClassDataLen; i++) {
         newClassData[i] = class_data[i];
         // Rewrite oo in class to aa
         if (i > 0 && class_data[i] == 'o' && class_data[i-1] == 'o') {
             newClassData[i] = newClassData[i-1] = 'a';
         }
     }

It's been so long since I've written C code, I wasn't expecting that the 
for loop couldn't declare int i.

Thanks!!
Coleen

> Thanks,
> Serguei
>
>
> On 10/5/17 11:15, coleen.phillimore at oracle.com wrote:
>> Summary: Don't double-free cached class bytes on redefinition loading 
>> failure.
>>
>> See bug for more description.  Tested with 
>> test/jdk/java/lang/instrument, 
>> test/hotspot/jtreg/runtime/RedefineTests, internal CDS tests, 
>> internal nsk.jvmti tests, and added test.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8178870.01/webrev
>> bug link https://bugs.openjdk.java.net/browse/JDK-8178870
>>
>> Thanks,
>> Coleen
>