[RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Weijun Wang
weijun.wang at oracle.com
Fri Aug 31 13:53:01 UTC 2018
> 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 security-dev
mailing list