> 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-dev@openjdk.java.net; core-libs-...@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-dev@openjdk.java.net; core-libs-...@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-dev@openjdk.java.net; core-libs-...@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-dev@openjdk.java.net; core-libs-...@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 > >>> > >