[RFR] 8205525 : Improve exception messages during manifest parsing of jar archives

[Baesken, Matthias]

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.wang at oracle.com>
> Sent: Freitag, 31. August 2018 15:53
> To: Baesken, Matthias <matthias.baesken at sap.com>
> Cc: Alan Bateman <Alan.Bateman at oracle.com>; Sean Mullan
> <sean.mullan at oracle.com>; Chris Hegarty <chris.hegarty at oracle.com>;
> security-dev at openjdk.java.net; core-libs-dev at 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.baesken at 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.wang at oracle.com>
> >> Sent: Freitag, 31. August 2018 04:32
> >> To: Baesken, Matthias <matthias.baesken at sap.com>
> >> Cc: Alan Bateman <Alan.Bateman at oracle.com>; Sean Mullan
> >> <sean.mullan at oracle.com>; Chris Hegarty <chris.hegarty at oracle.com>;
> >> security-dev at openjdk.java.net; core-libs-dev at 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.baesken at 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.wang at oracle.com>
> >>>> Sent: Donnerstag, 30. August 2018 16:04
> >>>> To: Baesken, Matthias <matthias.baesken at sap.com>
> >>>> Cc: Alan Bateman <Alan.Bateman at oracle.com>; Sean Mullan
> >>>> <sean.mullan at oracle.com>; Chris Hegarty
> <chris.hegarty at oracle.com>;
> >>>> security-dev at openjdk.java.net; core-libs-dev at 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.baesken at 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
> >