Hi Matthias The change looks fine. We can enhance the name if we want to support .SF parsing later.
Please revise your CSR and get it approved first. Thanks Max > On Sep 3, 2018, at 10:19 PM, Baesken, Matthias <matthias.baes...@sap.com> > wrote: > > Hi Max, I > > - moved getErrorPosition method to Manifest.java > - in read() method, removed "int offset" > - in the exception message, I write now " manifest of " ... (without > mentioning a manifest name) > > http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.8/ > > > Best regards, Matthias > > >> -----Original Message----- >> From: Weijun Wang <weijun.w...@oracle.com> >> Sent: Freitag, 31. August 2018 15:53 >> To: Baesken, Matthias <matthias.baes...@sap.com> >> Cc: Alan Bateman <alan.bate...@oracle.com>; Sean Mullan >> <sean.mul...@oracle.com>; Chris Hegarty <chris.hega...@oracle.com>; >> security-...@openjdk.java.net; core-libs-dev@openjdk.java.net >> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest >> parsing of jar archives >> >> >> >>> On Aug 31, 2018, at 8:52 PM, Baesken, Matthias >> <matthias.baes...@sap.com> wrote: >>> >>> Hi Max : >>> >>>> >>>> - No need to "import java.security.Security". >>> >>> Sure I can remove this, it is a leftover. >>> >>>> - In the updated read() method, I think there is no need to use an "int >> offset" >>>> parameter. "int lineNumber" is enough and you can modify it and return it >>>> without a new local variable. >>>> >>> >>> Currently we need it in Manifest.java public void read(InputStream is) >> throws IOException { ... } >> >> I was talking about the name of the parameter. You can simply use >> >> int read(Manifest.FastInputStream is, byte[] lbuf, String filename, int >> lineNumber) >> >> and there is no need to reassign it with "int lineNumber = offset" anymore. >> >>> >>>> In fact, I have a small concern on the hardcoded file name here, because >>>> when verifying a signed jar, the META-INF/X.SF file is also parsed into a >>>> Manifest object and if it's invalid similar exceptions will be thrown. I >>>> don't >>>> have a nice solution now. >>> >>> Then lets just say <jarfile>:<line-number> (e.g. test.jar:10 ) >>> >>> Or ( to mention that it is about a manifest from the jar ) >>> >>> <jarfile>:manifest-line <line-number> (e.g. test.jar:manifest-line 10 ) >> >> How about you pass in the full name ("/path/to/file.jar!META- >> INF/MANIFEST.MF") to "new Manifest(stream,name)" directly? >> >> So the name will be constructed in JarFile.java (after checking for >> jarPathInExceptionText). The getErrorPosition method simply concat the >> name (if not null) and the line number. Thus the exception thrown from >> parsing X.SF simply will not include any file info. If we want it we can >> enhance >> later. >> >> Thanks >> Max >> >>> >>> >>> Best regards, Matthias >>> >>> >>> >>>> -----Original Message----- >>>> From: Weijun Wang <weijun.w...@oracle.com> >>>> Sent: Freitag, 31. August 2018 04:32 >>>> To: Baesken, Matthias <matthias.baes...@sap.com> >>>> Cc: Alan Bateman <alan.bate...@oracle.com>; Sean Mullan >>>> <sean.mul...@oracle.com>; Chris Hegarty <chris.hega...@oracle.com>; >>>> security-...@openjdk.java.net; core-libs-dev@openjdk.java.net >>>> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest >>>> parsing of jar archives >>>> >>>> Some more comments: >>>> >>>> Attributes.java >>>> >>>> - No need to "import java.security.Security". >>>> >>>> - In the updated read() method, I think there is no need to use an "int >> offset" >>>> parameter. "int lineNumber" is enough and you can modify it and return it >>>> without a new local variable. >>>> >>>> - I feel like calling Attributes::getErrorPosition from Manifest a little >> strange. >>>> Maybe it's better to define the method in Manifest and call it from >>>> Attributes. First, I think putting the method in a higher level object >>>> makes >>>> more sense. Second, the position is for the whole Manifest and not an >>>> Attributes section (I mean the line number is counted from the first line >>>> of >>>> the manifest). >>>> >>>>> On Aug 30, 2018, at 10:50 PM, Baesken, Matthias >>>> <matthias.baes...@sap.com> wrote: >>>>> >>>>> >>>>> >>>>> Hi Max, probably we should add the info about the MANIFEST.MF , >> for >>>> example : >>>>> change getErrorPosition to >>>>> >>>>> >>>> >> http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.7/src/java.base/s >>>> hare/classes/java/util/jar/Attributes.java.udiff.html >>>>> >>>>> >>>>> static String getErrorPosition(String filename, final int lineNumber) { >>>>> if (filename == null || !jarPathInExceptionText) { >>>>> return "META-INF/MANIFEST.MF line:" + lineNumber; >>>>> } >>>>> >>>>> final File file = new File(filename); >>>>> return AccessController.doPrivileged(new PrivilegedAction<String>() >> { >>>>> public String run() { >>>>> return file.getAbsolutePath() + "!META-INF/MANIFEST.MF line:" >> + >>>> lineNumber; >>>> >>>> I prefer "file:line" which is more compact and more commonly used. >>>> >>>> In fact, I have a small concern on the hardcoded file name here, because >>>> when verifying a signed jar, the META-INF/X.SF file is also parsed into a >>>> Manifest object and if it's invalid similar exceptions will be thrown. I >>>> don't >>>> have a nice solution now. if we want to show the .SF name also, we will >> need >>>> a public API because SignatureFileVerifier.java is inside >>>> sun.security.util. >>>> Something like Manifest(InputStream,jarName,entryName)? >>>> >>>> Sorry for making this complicated. >>>> >>>> Thanks >>>> Max >>>> >>>>> } >>>>> ..... >>>>> >>>>> >>>>> Best regards, Matthias >>>>> >>>>> >>>>> >>>>>> -----Original Message----- >>>>>> From: Weijun Wang <weijun.w...@oracle.com> >>>>>> Sent: Donnerstag, 30. August 2018 16:04 >>>>>> To: Baesken, Matthias <matthias.baes...@sap.com> >>>>>> Cc: Alan Bateman <alan.bate...@oracle.com>; Sean Mullan >>>>>> <sean.mul...@oracle.com>; Chris Hegarty >> <chris.hega...@oracle.com>; >>>>>> security-...@openjdk.java.net; core-libs-dev@openjdk.java.net >>>>>> Subject: Re: [RFR] 8205525 : Improve exception messages during >> manifest >>>>>> parsing of jar archives >>>>>> >>>>>> >>>>>> >>>>>>> On Aug 30, 2018, at 8:26 PM, Baesken, Matthias >>>>>> <matthias.baes...@sap.com> wrote: >>>>>>> >>>>>>>> - What will the output look like? Is it "/tmp/x.jar:100"? >>>>>>>> >>>>>>> >>>>>>> Yes it look like this : >>>>>>> >>>>>>> line too long (/testdata/jars/file_with_long_line_1.jar:2) >>>>>> >>>>>> Is this a little misleading? I think you mean >>>>>> >>>>>> /testdata/jars/file_with_long_line_1.jar!META-INF/MANIFEST.MF:2 >>>>>> >>>>>> Thanks >>>>>> Max >>> >
