> On Aug 31, 2018, at 8:52 PM, Baesken, Matthias <matthias.baes...@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.w...@oracle.com>
>> Sent: Freitag, 31. August 2018 04:32
>> To: Baesken, Matthias <matthias.baes...@sap.com>
>> Cc: Alan Bateman <alan.bate...@oracle.com>; Sean Mullan
>> <sean.mul...@oracle.com>; Chris Hegarty <chris.hega...@oracle.com>;
>> security-...@openjdk.java.net; core-libs-dev@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.baes...@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.w...@oracle.com>
>>>> Sent: Donnerstag, 30. August 2018 16:04
>>>> To: Baesken, Matthias <matthias.baes...@sap.com>
>>>> Cc: Alan Bateman <alan.bate...@oracle.com>; Sean Mullan
>>>> <sean.mul...@oracle.com>; Chris Hegarty <chris.hega...@oracle.com>;
>>>> security-...@openjdk.java.net; core-libs-dev@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.baes...@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
>
