[RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Weijun Wang
weijun.wang at oracle.com
Tue Sep 4 13:31:58 UTC 2018
I've added myself as a reviewer. You might want to set scope to "JDK".
A CSR is approved by the CSR group after you finalize it. First you should propose it. If you think it's urgent or trivial, you can also fast-track it.
Read https://wiki.openjdk.java.net/display/csr/Main for more details.
Thanks
Max
> On Sep 4, 2018, at 7:33 PM, Baesken, Matthias <matthias.baesken at sap.com> wrote:
>
>> 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