RFR: 8242891: vmTestbase/nsk/jvmti/ test should be fixed to fail early if JVMTI function return error

[Leonid Mesnik]

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/20200611/39bb2a10/attachment.htm>