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

[Sean Mullan]

I need more time to finish my review but here are some initial comments:

- java.security

1079 #  jarpath - enables more detailed information in the IOExceptions 
thrown

Use "jarPath" to be consistent with "hostInfo".

1080 #            by java.util.jar.Attributes  and java.util.jar.Manifest

and java.util.jar.JarFile

- Manifest.java

57     private String jarFilename = null;

Not necessary to set it to null, as that is the default.

   82     Manifest(InputStream is, String jarFilename) throws IOException {
   83         this.jarFilename = jarFilename;
   84         read(is);
   85     }

Read from the InputStream and check for error conditions *before* 
setting jarFilename (switch lines 83 & 84). This is a general best 
practice but can also protect against finalizer attacks. See JSCG 7-3 
for more info: 
https://www.oracle.com/technetwork/java/seccodeguide-139067.html#7

- Attributes.java

As Chris and Alan mentioned, you should move the parsing of the property 
to a more general location so it can be used by other code that uses 
this property.

--Sean

On 8/8/18 11:00 AM, Sean Mullan wrote:
> Cross-posting to security-dev since this fix is security related and 
> should also be reviewed there.
> 
> --Sean
> 
> On 8/7/18 11:00 AM, Baesken, Matthias wrote:
>> Ping ....  😊  , any reviews / comments ?
>>
>> Thanks , Matthias
>>
>>> -----Original Message-----
>>> From: Baesken, Matthias
>>> Sent: Dienstag, 31. Juli 2018 12:28
>>> To: 'Chris Hegarty' <chris.hegarty at oracle.com>; Alan Bateman
>>> <Alan.Bateman at oracle.com>
>>> Cc: core-libs-dev at openjdk.java.net; Lindenmaier, Goetz
>>> <goetz.lindenmaier at sap.com>; Langer, Christoph
>>> <christoph.langer at sap.com>
>>> Subject: RE: [RFR] 8205525 : Improve exception messages during manifest
>>> parsing of jar archives
>>>
>>> Hello ,
>>> looks like  the  generalization of  the `includeInExceptions` 
>>> security   property
>>> is now in jdk/jdk  after
>>>
>>> "8207846: Generalize the jdk.net.includeInExceptions security property"
>>>
>>> was added, great news  and thanks to Chris for driving this !
>>>
>>> I use this security property now as well , and updated the  change :
>>>
>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.3/
>>>
>>> I updated the CSR as well :
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8207768
>>>
>>>
>>> Best regards, Matthias
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Chris Hegarty <chris.hegarty at oracle.com>
>>>> Sent: Donnerstag, 19. Juli 2018 14:54
>>>> To: Alan Bateman <Alan.Bateman at oracle.com>; Baesken, Matthias
>>>> <matthias.baesken at sap.com>
>>>> Cc: core-libs-dev at openjdk.java.net; Lindenmaier, Goetz
>>>> <goetz.lindenmaier at sap.com>
>>>> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest
>>>> parsing of jar archives
>>>>
>>>>
>>>>> On 19 Jul 2018, at 11:46, Alan Bateman <Alan.Bateman at oracle.com>
>>>> wrote:
>>>>>
>>>>> On 19/07/2018 09:07, Baesken, Matthias wrote:
>>>>>> Hello, in the meantime I  prepared a CSR :
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8207768
>>>>>>
>>>>>>
>>>>>>> jdk.includeInExceptions expands the scope. That might be okay but we
>>>>>>> will need to re-visit jdk.net.includeInExceptions and also move the
>>>>>>> support to somewhere in jdk.internal so that it can be used by other
>>>>>>> code in java.base.
>>>>>> Is there some support code for  " jdk.net.includeInExceptions "    
>>>>>> or do
>>>> you just  mean  the name of the property ?
>>>>>>
>>>>> Chris is right that it's a bit premature to submit a CSR while the 
>>>>> question
>>> on
>>>> whether to rename the existing security property is on the table. I 
>>>> have no
>>>> objection to doing that.
>>>>
>>>> I filed the following issue to generalize the `includeInExceptions` 
>>>> security
>>>>   property:
>>>>    https://bugs.openjdk.java.net/browse/JDK-8207846
>>>>
>>>> I would like to resolve 8207846 first, then 8205525 can propose to 
>>>> add the
>>>> new argument value, `jarPath`.
>>>>
>>>> -Chris.