I've updated the test, switched to an in-memory class loader, and added a test case. Please review.
On 10/01/13 16:27, Eric McCorkle wrote: > On 10/01/13 02:41, Joe Darcy wrote: > > (Suggested changes have been applied) > >> >> I think the test is acceptable as-is, but an RFE could be filed for some >> refactoring (having each bad class be represented as a diff from a base >> byte[], avoiding sending the bytes through the file system). >> > > Better yet: I've filed an RFE for a framework for generating bad class > files. > > Webrev has been updated, please review: > http://cr.openjdk.java.net/~emc/8020981/ > >> Cheers, >> >> -Joe >> >> On 9/24/2013 12:15 PM, Eric McCorkle wrote: >>> Webrev updated to address these issues. >>> >>> On 09/24/13 07:51, Joel Borggren-Franck wrote: >>> >>>> 364 try { >>>> 365 tmp = getParameters0(); >>>> 366 } catch(IllegalArgumentException e) { >>>> 367 // Rethrow ClassFormatErrors >>>> 368 throw new MalformedParametersException("Invalid >>>> constant pool index"); >>>> 369 } >>>> >>>> What is causing the IAE? Have you considered having >>>> MalformedParametersExcepton taking a Throwable cause and having the IAE >>>> as cause? >>>> >>> The IAE comes from hotspot itself. There is a standard constant pool >>> index verifying function that throws IAE if given a bad index. >>> >>>> On 2013-09-19, Eric McCorkle wrote: >>>>> The webrev has been updated with Joe's comments addressed. >>>>> >>>>> On 09/19/13 00:11, David Holmes wrote: >>>>>> On 19/09/2013 9:59 AM, Eric McCorkle wrote: >>>>>>> This still needs to be reviewed. >>>>>> You seem to have ignored Joe's comments regarding the change to >>>>>> Modifier >>>>>> being incorrect. >>>>>> >>>>>> David >>>>>> >>>>>>> On 09/16/13 14:50, Eric McCorkle wrote: >>>>>>>> I pulled the class files into byte array constants, as a temporary >>>>>>>> measure until a viable method for testing bad class files is >>>>>>>> developed. >>>>>>>> >>>>>>>> The webrev has been refreshed. The class files will be taken out >>>>>>>> before >>>>>>>> I push. >>>>>>>> >>>>>>>> http://cr.openjdk.java.net/~emc/8020981/ >>>>>>>> >>>>>>>> On 09/13/13 20:48, Joe Darcy wrote: >>>>>>>>> To avoid storing binaries in Hg, you could try something like: >>>>>>>>> >>>>>>>>> * uuencode / ascii armor the class file >>>>>>>>> * convert to byte array in the test >>>>>>>>> * load classes from byte array >>>>>>>>> >>>>>>>>> -Joe >>>>>>>>> >>>>>>>>> On 09/13/2013 11:54 AM, Eric McCorkle wrote: >>>>>>>>>> I did it by hand with emacs. >>>>>>>>>> >>>>>>>>>> I would really rather tackle the bad class files for testing issue >>>>>>>>>> once >>>>>>>>>> and for all, the Right Way (tm). But with ZBB looming, now is >>>>>>>>>> not the >>>>>>>>>> time to do it. >>>>>>>>>> >>>>>>>>>> Hence, I have created this task >>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8024674 >>>>>>>>>> >>>>>>>>>> I also just created this one: >>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8024812 >>>>>>>>>> >>>>>>>>>> On 09/13/13 13:54, Peter Levart wrote: >>>>>>>>>>> Hi Eric, >>>>>>>>>>> >>>>>>>>>>> How did you create those class files? By hand using a HEX >>>>>>>>>>> editor? Did >>>>>>>>>>> you create a program that patched the original class file? If the >>>>>>>>>>> later >>>>>>>>>>> is the case, you could pack that patching logic inside a custom >>>>>>>>>>> ClassLoader... >>>>>>>>>>> >>>>>>>>>>> To hacky? Dependent on future changes of javac? At least the "bad >>>>>>>>>>> name" >>>>>>>>>>> patching could be performed trivially and reliably, I think: >>>>>>>>>>> search and >>>>>>>>>>> replace with same-length string. >>>>>>>>>>> >>>>>>>>>>> Regards, Peter >>>>>>>>>>> >>>>>>>>>>> On 09/13/2013 07:35 PM, Eric McCorkle wrote: >>>>>>>>>>>> Ugh. Byte arrays of class file data is really a horrible >>>>>>>>>>>> solution. >>>>>>>>>>>> >>>>>>>>>>>> I have already filed a task for test development post ZBB to >>>>>>>>>>>> develop a >>>>>>>>>>>> solution for generating bad class files. I'd prefer to file a >>>>>>>>>>>> follow-up >>>>>>>>>>>> to this to add the bad class file tests when that's done. >>>>>>>>>>>> >>>>>>>>>>>> On 09/13/13 10:55, Joel Borggrén-Franck wrote: >>>>>>>>>>>>> I think the right thing to do is to include the original >>>>>>>>>>>>> compiling >>>>>>>>>>>>> source in a comment, together with a comment on how you modify >>>>>>>>>>>>> them, and then the result as a byte array. >>>>>>>>>>>>> >>>>>>>>>>>>> IIRC I have seen test of that kind before somewhere in our >>>>>>>>>>>>> repo. >>>>>>>>>>>>> >>>>>>>>>>>>> cheers >>>>>>>>>>>>> /Joel >>>>>>>>>>>>> >>>>>>>>>>>>> On Sep 13, 2013, at 4:49 PM, Eric McCorkle >>>>>>>>>>>>> <eric.mccor...@oracle.com> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>> There is no simple means of generating bad class files for >>>>>>>>>>>>>> testing. >>>>>>>>>>>>>> This is a huge deficiency in our testing abilities. >>>>>>>>>>>>>> >>>>>>>>>>>>>> If these class files shouldn't go in, then I'm left with no >>>>>>>>>>>>>> choice >>>>>>>>>>>>>> but >>>>>>>>>>>>>> to check in no test for this patch. >>>>>>>>>>>>>> >>>>>>>>>>>>>> However, anyone can run the test I've provided with the class >>>>>>>>>>>>>> files and >>>>>>>>>>>>>> see that it works. >>>>>>>>>>>>>> >>>>>>>>>>>>>> On 09/13/13 09:55, Joel Borggrén-Franck wrote: >>>>>>>>>>>>>>> Hi Eric, >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> IIRC we don't check in classfiles into the repo. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> I'm not sure how we handle testing of broken class-files >>>>>>>>>>>>>>> in jdk, >>>>>>>>>>>>>>> but ASM might be an option, or storing the class file as an >>>>>>>>>>>>>>> embedded byte array in the test. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> cheers >>>>>>>>>>>>>>> /Joel >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> On Sep 13, 2013, at 3:40 PM, Eric McCorkle >>>>>>>>>>>>>>> <eric.mccor...@oracle.com> wrote: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> A new webrev is posted (and crucible updated), which >>>>>>>>>>>>>>>> actually >>>>>>>>>>>>>>>> validates >>>>>>>>>>>>>>>> parameter names correctly. Apologies for the last one. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> On 09/12/13 16:02, Eric McCorkle wrote: >>>>>>>>>>>>>>>>> Hello, >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Please review this patch, which implements correct >>>>>>>>>>>>>>>>> behavior for >>>>>>>>>>>>>>>>> the >>>>>>>>>>>>>>>>> Parameter Reflection API in the case of malformed class >>>>>>>>>>>>>>>>> files. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> The bug report is here: >>>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8020981 >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> The webrev is here: >>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~emc/8020981/ >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> This review is also on crucible. The ID is CR-JDK8TL-182. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Thanks, >>>>>>>>>>>>>>>>> Eric >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>> <eric_mccorkle.vcf> >>
