RFR: 8242891: vmTestbase/nsk/jvmti/ test should be fixed to fail early if JVMTI function return error
Leonid Mesnik
leonid.mesnik at oracle.com
Fri Jun 12 23:18:18 UTC 2020
Fixed all places, updated copyright. Still need second review
http://cr.openjdk.java.net/~lmesnik/8242891/webrev.02/
Leonid
On 6/11/20 8:41 PM, serguei.spitsyn at oracle.com wrote:
> Hi Leonid,
>
> It is much better now.
>
> Several places still need the same fix.
>
> http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetAllThreads/allthr001/allthr001.cpp.frames.html
>
> 211 for (i = 0; i < thrInfo[ind].cnt; i++) {
> 212 for (j = 0, found = 0; j < threadsCount && !found; j++) {
> 213 err = jvmti->GetThreadInfo(threads[j], &inf);
> 214 if (err != JVMTI_ERROR_NONE) {
> 215 printf("Failed to get thread info: %s (%d)\n",
> 216 TranslateError(err), err);
> 217 result = STATUS_FAILED;
> 218 }
> 219 if (printdump == JNI_TRUE) {
> 220 printf(" >>> %s", inf.name);
> 221 }
> 222 found = (inf.name != NULL &&
> 223 strstr(inf.name, thrInfo[ind].thrNames[i]) == inf.name &&
> 224 (ind == 4 || strlen(inf.name) ==
> 225 strlen(thrInfo[ind].thrNames[i])));
> 226 }
> A return is needed after line 217, otherwise the the inf value is used
> at lines 222-224.
>
> http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetBytecodes/bytecodes003/bytecodes003.cpp.frames.html
>
> A return is needed for the errors:
> 363 result = STATUS_FAILED;
> 372 result = STATUS_FAILED;
> 384 result = STATUS_FAILED;
>
> http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/MethodEntry/mentry001/mentry001.cpp.frames.html
>
> A return is needed for the errors:
> 82 result = STATUS_FAILED;
> 94 result = STATUS_FAILED;
> 100 result = STATUS_FAILED;
>
> http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/MethodExit/mexit001/mexit001.cpp.frames.html
>
> A return is needed for the error:
> 98 result = STATUS_FAILED;
>
> http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/MethodExit/mexit002/mexit002.cpp.frames.html
>
> A return is needed for the error:
> 98 result = STATUS_FAILED;
>
> http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/RedefineClasses/redefclass019/redefclass019.cpp.frames.html
>
> A return is needed for the error:
> 186 result = STATUS_FAILED;
>
> Also, I do not like many uninitialized locals in these tests.
> But it is for another pass.
>
> Otherwise, it looks good.
> No need for another webrev if you fix the above.
> I hope, you will update copyright comments before push.
>
> Thanks,
> Serguei
>
>
> On 6/11/20 15:30, Leonid Mesnik wrote:
>>
>> Agree, it would be better to don't try to use data from functions
>> with error code. The new webrev:
>>
>> http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/
>>
>> I tried to prevent any usage of possibly corrupted data. Mostly
>> strings or allocated data, sometimes method/class id which are used
>> my other JVMTI functions.
>>
>> Leonid
>>
>> On 6/9/20 6:59 PM, serguei.spitsyn at oracle.com wrote:
>>> On 6/9/20 12:58, Leonid Mesnik wrote:
>>>>
>>>> Hi
>>>>
>>>>
>>>> On 6/9/20 12:34 PM, serguei.spitsyn at oracle.com wrote:
>>>>> Hi Leonid,
>>>>>
>>>>> Thank you for taking care about this!
>>>>> It looks good in general.
>>>>> However, I think, a similar return is needed in more cases.
>>>>>
>>>>> One example:
>>>>>
>>>>> http://cr.openjdk.java.net/~lmesnik/8242891/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/Exception/exception001/exception001.cpp.frames.html
>>>>>
>>>>> 99 err = jvmti_env->GetMethodDeclaringClass(method, &cls);
>>>>> 100 if (err != JVMTI_ERROR_NONE) {
>>>>> 101 printf("(GetMethodDeclaringClass#t) unexpected error: %s (%d)\n",
>>>>> 102 TranslateError(err), err);
>>>>> 103 result = STATUS_FAILED;
>>>>> 104 return;
>>>>> 105 }
>>>>> 106 err = jvmti_env->GetClassSignature(cls, &ex.t_cls, &generic);
>>>>> 107 if (err != JVMTI_ERROR_NONE) {
>>>>> 108 printf("(GetClassSignature#t) unexpected error: %s (%d)\n",
>>>>> 109 TranslateError(err), err);
>>>>> 110 result = STATUS_FAILED;
>>>>> 111 }
>>>>> 112 err = jvmti_env->GetMethodName(method,
>>>>> 113 &ex.t_name, &ex.t_sig, &generic);
>>>>> 114 if (err != JVMTI_ERROR_NONE) {
>>>>> 115 printf("(GetMethodName#t) unexpected error: %s (%d)\n",
>>>>> 116 TranslateError(err), err);
>>>>> 117 result = STATUS_FAILED;
>>>>> 118 }
>>>>> 119 ex.t_loc = location;
>>>>> 120 err = jvmti_env->GetMethodDeclaringClass(catch_method, &cls);
>>>>> 121 if (err != JVMTI_ERROR_NONE) {
>>>>> 122 printf("(GetMethodDeclaringClass#c) unexpected error: %s (%d)\n",
>>>>> 123 TranslateError(err), err);
>>>>> 124 result = STATUS_FAILED;
>>>>> 125 return;
>>>>> 126 }
>>>>> 127 err = jvmti_env->GetClassSignature(cls, &ex.c_cls, &generic);
>>>>> 128 if (err != JVMTI_ERROR_NONE) {
>>>>> 129 printf("(GetClassSignature#c) unexpected error: %s (%d)\n",
>>>>> 130 TranslateError(err), err);
>>>>> 131 result = STATUS_FAILED;
>>>>> 132 }
>>>>> 133 err = jvmti_env->GetMethodName(catch_method,
>>>>> 134 &ex.c_name, &ex.c_sig, &generic);
>>>>> 135 if (err != JVMTI_ERROR_NONE) {
>>>>> 136 printf("(GetMethodName#c) unexpected error: %s (%d)\n",
>>>>> 137 TranslateError(err), err);
>>>>> 138 result = STATUS_FAILED;
>>>>> 139 }
>>>>>
>>>>> In the fragment above you added return for JVMTI
>>>>> GetMethodDeclaringClass error.
>>>>> But GetMethodName and GetClassSignature can be also problematic as
>>>>> the returned names are printed below.
>>>>> It seems to be more safe and even simpler to add returns for such
>>>>> cases as well.
>>>>> Otherwise, the code reader is puzzled why there is a return in one
>>>>> failure case and there is no such return in another.
>>>>
>>>> It is a good question if we want to fix such places or even fails
>>>> with first JVMTI failure. (I even started to fix it in the such way
>>>> but find that existing tests usually don't fail always).
>>>>
>>>
>>> I do not suggest to fix all the tests but those which you are
>>> already fixing.
>>>
>>>
>>>> The difference is that test tries to reuse "cls" in other JVMTI
>>>> function and going to generate very misleading crash. How it just
>>>> tries to compare ex and exs values. So test might crash but clearly
>>>> outside of JVMTI function and with some useful info. So I am not
>>>> sure if fixing these lines improve test failure handling.
>>>>
>>>
>>> If JVMTI functions fail with an error code the results with symbolic
>>> strings must be considered invalid.
>>> However, they are used later (the values are printed).
>>> It is better to bail out in such cases.
>>> It should not be a problem to add similar returns in such cases.
>>> Or do you think it is important to continue execution for some reason?
>>>
>>>> Assuming that most of existing tests fails early only if going to
>>>> re-use possible corrupted data I propose to fix this separately. We
>>>> need to figure out when to fail or to try to finish.
>>>>
>>>
>>> Do you suggest it for the updated tests only or for all the tests
>>> with such problems?
>>>
>>> Thanks,
>>> Serguei
>>>
>>>> Leonid
>>>>
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>
>>>>> On 6/1/20 21:33, Leonid Mesnik wrote:
>>>>>> Hi
>>>>>>
>>>>>> Could you please review following fix which stop test execution
>>>>>> if JVMTI function returns error. The test fails anyway however
>>>>>> using potentially bad data in JVMTI function might cause
>>>>>> misleading crash failures. The hs_err will contains the
>>>>>> stacktrace not with problem function but with function called
>>>>>> with corrupted data. Most of tests already has such behavior but
>>>>>> not all. Also I fixed a couple of tests to finish if they haven't
>>>>>> managed to suspend thread.
>>>>>>
>>>>>> I've updated only tests which try to use corrupted data in JVMTI
>>>>>> functions after errors. I haven't updated tests which just
>>>>>> compare/print values from erroring JVMTI functions. The crash in
>>>>>> strcmp/println is not so misleading and might be point to real
>>>>>> issue.
>>>>>>
>>>>>> webrev: http://cr.openjdk.java.net/~lmesnik/8242891/webrev.00/
>>>>>>
>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8242891
>>>>>>
>>>>>> Leonid
>>>>>>
>>>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20200612/4111ba74/attachment-0001.htm>
More information about the serviceability-dev
mailing list