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

[Weijun Wang]

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