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 > >
