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

Baesken, Matthias matthias.baesken at sap.com
Tue Sep 4 11:33:44 UTC 2018


> 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.wang at oracle.com>
> Sent: Montag, 3. September 2018 17:14
> 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
> 
> 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.baesken at 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.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
> >>>
> >



More information about the core-libs-dev mailing list