I've added myself as a reviewer. You might want to set scope to "JDK".
A CSR is approved by the CSR group after you finalize it. First you should propose it. If you think it's urgent or trivial, you can also fast-track it. Read https://wiki.openjdk.java.net/display/csr/Main for more details. Thanks Max > On Sep 4, 2018, at 7:33 PM, Baesken, Matthias <matthias.baes...@sap.com> > wrote: > >> 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. > > Hi Max, Thanks ! > > I adjusted the CSR , please approve : > > https://bugs.openjdk.java.net/browse/JDK-8207768 > > Best regards, Matthias > > > >> -----Original Message----- >> From: Weijun Wang <weijun.w...@oracle.com> >> Sent: Montag, 3. September 2018 17:14 >> 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 >> >> 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 >>>>> >>> >