Hi Eric,
Some feedback:
Executable.java:
299 * (i) The number of parameters (parameter_count) is wrong for the
method
What is wrong in this case? Do you mean inconsistent with the signature?
302 * (iv) A parameter's name is "", or contains an illegal character [0]
What does [0] mean in this case? A placeholder for an in-mail reference
or a null byte in a modified utf8 array?
299 * (i) The number of parameters (parameter_count) is wrong for the
method
300 * (ii) A constant pool index is out of bounds.
301 * (iii) A constant pool index does not refer to a UTF-8 entry
302 * (iv) A parameter's name is "", or contains an illegal character [0]
303 * (v) The flags field contains an illegal flag (something other than
304 * FINAL, SYNTHETIC, or MANDATED)
The new "markup" looks odd. I think you should use <ul> or <ol> to be
consistent j.l.Class. See Class.getMethods() for an example.
306 * @throws MalformedParametersException if the class file contains
307 * a MethodParameters attribute that is improperly formatted.
308 * @return an array of {@code Parameter} objects representing all
309 * the parameters to the executable this object represents
@throws ends with a period, @return does not. I'm not sure what the
convention is but I think you want to be consistent.
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?
MalformedParametersException.java:
42 /**
43 * Use serialVersionUID from JDK 1.1.X for interoperability
44 */
45 private static final long serialVersionUID = 20130919L;
Was this type present in 1.1.x? This comment makes no sense to me.
Please explain.
BadClassFiles.java:
Thanks for encoding the class-files. Please include the original source
above the bytes.
cheers
/Joel
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>
> >>>>
