Could I have 2nd reviewer? Leonid
> On Jun 25, 2020, at 10:58 AM, Leonid Mesnik <leonid.mes...@oracle.com> wrote: > > Ping > >> On Jun 12, 2020, at 4:18 PM, Leonid Mesnik <leonid.mes...@oracle.com >> <mailto:leonid.mes...@oracle.com>> wrote: >> >> Fixed all places, updated copyright. Still need second review >> >> http://cr.openjdk.java.net/~lmesnik/8242891/webrev.02/ >> <http://cr.openjdk.java.net/~lmesnik/8242891/webrev.02/> >> Leonid >> >> On 6/11/20 8:41 PM, serguei.spit...@oracle.com >> <mailto:serguei.spit...@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 >>> >>> <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 >>> >>> <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 >>> >>> <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 >>> >>> <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 >>> >>> <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 >>> >>> <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/ >>>> <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.spit...@oracle.com >>>> <mailto:serguei.spit...@oracle.com> wrote: >>>>> On 6/9/20 12:58, Leonid Mesnik wrote: >>>>>> Hi >>>>>> >>>>>> >>>>>> On 6/9/20 12:34 PM, serguei.spit...@oracle.com >>>>>> <mailto:serguei.spit...@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 >>>>>>> >>>>>>> <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/ >>>>>>>> <http://cr.openjdk.java.net/~lmesnik/8242891/webrev.00/> >>>>>>>> >>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8242891 >>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8242891> >>>>>>>> >>>>>>>> Leonid >>>>>>>> >>>>>>> >>>>> >>> >
