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

classic Classic list List threaded Threaded
51 messages Options
123
Reply | Threaded
Open this post in threaded view
|

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

Sean Mullan
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' <[hidden email]>; Alan Bateman
>> <[hidden email]>
>> Cc: [hidden email]; Lindenmaier, Goetz
>> <[hidden email]>; Langer, Christoph
>> <[hidden email]>
>> 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 <[hidden email]>
>>> Sent: Donnerstag, 19. Juli 2018 14:54
>>> To: Alan Bateman <[hidden email]>; Baesken, Matthias
>>> <[hidden email]>
>>> Cc: [hidden email]; Lindenmaier, Goetz
>>> <[hidden email]>
>>> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest
>>> parsing of jar archives
>>>
>>>
>>>> On 19 Jul 2018, at 11:46, Alan Bateman <[hidden email]>
>>> 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.
Reply | Threaded
Open this post in threaded view
|

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

Alan Bateman
On 07/08/2018 16:00, Baesken, Matthias wrote:
> Ping ....  😊  , any reviews / comments ?
Did we get to a conclusion on whether to have central infrastructure to
read/parse the security property? As I recall, this one was originally
proposed before the generalization of the networking solution. There are
details related to trimming that would be better not to duplicate if
possible.

Also, I think one of my comments on the original patch is that we should
get the style and line lengths a bit more consistent with the existing
code (the patch added excessively long lines for example).

-Alan.
Reply | Threaded
Open this post in threaded view
|

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

Baesken, Matthias
> Did we get to a conclusion on whether to have central infrastructure to
> read/parse the security property? As I recall, this one was originally
> proposed before the generalization of the networking solution. There are
> details related to trimming that would be better not to duplicate if
> possible.

Hi Alan / Chris ,

I am aware of  

https://bugs.openjdk.java.net/browse/JDK-8207846

where jdk.net.includeInExceptions   has been generalized to be used for other use cases than  exceptions in the networking area .
This  has been pushed in the meantine.

Could you point me to the other discussion, is there already a webrev posted for this ?

>
> Also, I think one of my comments on the original patch is that we should
> get the style and line lengths a bit more consistent with the existing
> code (the patch added excessively long lines for example).
>

Sure I'll look into it !

Best regards, Matthias


> -----Original Message-----
> From: Alan Bateman <[hidden email]>
> Sent: Mittwoch, 8. August 2018 20:30
> To: Baesken, Matthias <[hidden email]>; Chris Hegarty
> <[hidden email]>
> Cc: [hidden email]; Lindenmaier, Goetz
> <[hidden email]>; Langer, Christoph
> <[hidden email]>; OpenJDK Dev list <security-
> [hidden email]>
> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest
> parsing of jar archives
>
> On 07/08/2018 16:00, Baesken, Matthias wrote:
> > Ping ....  😊  , any reviews / comments ?
> Did we get to a conclusion on whether to have central infrastructure to
> read/parse the security property? As I recall, this one was originally
> proposed before the generalization of the networking solution. There are
> details related to trimming that would be better not to duplicate if
> possible.
>
> Also, I think one of my comments on the original patch is that we should
> get the style and line lengths a bit more consistent with the existing
> code (the patch added excessively long lines for example).
>
> -Alan.
Reply | Threaded
Open this post in threaded view
|

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

Chris Hegarty
Matthias,

> On 9 Aug 2018, at 08:07, Baesken, Matthias <[hidden email]> wrote:
>
>> Did we get to a conclusion on whether to have central infrastructure to
>> read/parse the security property? As I recall, this one was originally
>> proposed before the generalization of the networking solution. There are
>> details related to trimming that would be better not to duplicate if
>> possible.
>
> Hi Alan / Chris ,
>
> I am aware of  
>
> https://bugs.openjdk.java.net/browse/JDK-8207846
>
> where jdk.net.includeInExceptions   has been generalized to be used for other use cases than  exceptions in the networking area .
> This  has been pushed in the meantine.
>
> Could you point me to the other discussion, is there already a webrev posted for this ?

Given that 8207846 was being targeted to JDK 11, during RDP 1,
it was decided to keep the changes as minimal as possible. The
supporting implementation, that reads and parses the property,
remains in src/java.base/share/classes/sun/net/util/SocketExceptions.java.
Alan referred to this in one of the review comments for 8207846 [1].
If this mechanism is to be used outside of the networking area,
and I think it can, then the implementation in SocketExceptions
should probably be moved to some other more appropriate
internal package.

>>
>> Also, I think one of my comments on the original patch is that we should
>> get the style and line lengths a bit more consistent with the existing
>> code (the patch added excessively long lines for example).
>>
>
> Sure I'll look into it !


-Chris.

[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-July/054501.html

> Best regards, Matthias
>
>
>> -----Original Message-----
>> From: Alan Bateman <[hidden email]>
>> Sent: Mittwoch, 8. August 2018 20:30
>> To: Baesken, Matthias <[hidden email]>; Chris Hegarty
>> <[hidden email]>
>> Cc: [hidden email]; Lindenmaier, Goetz
>> <[hidden email]>; Langer, Christoph
>> <[hidden email]>; OpenJDK Dev list <security-
>> [hidden email]>
>> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest
>> parsing of jar archives
>>
>> On 07/08/2018 16:00, Baesken, Matthias wrote:
>>> Ping ....  😊  , any reviews / comments ?
>> Did we get to a conclusion on whether to have central infrastructure to
>> read/parse the security property? As I recall, this one was originally
>> proposed before the generalization of the networking solution. There are
>> details related to trimming that would be better not to duplicate if
>> possible.
>>
>> Also, I think one of my comments on the original patch is that we should
>> get the style and line lengths a bit more consistent with the existing
>> code (the patch added excessively long lines for example).
>>
>> -Alan.

Reply | Threaded
Open this post in threaded view
|

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

Sean Mullan
In reply to this post by 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' <[hidden email]>; Alan Bateman
>>> <[hidden email]>
>>> Cc: [hidden email]; Lindenmaier, Goetz
>>> <[hidden email]>; Langer, Christoph
>>> <[hidden email]>
>>> 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 <[hidden email]>
>>>> Sent: Donnerstag, 19. Juli 2018 14:54
>>>> To: Alan Bateman <[hidden email]>; Baesken, Matthias
>>>> <[hidden email]>
>>>> Cc: [hidden email]; Lindenmaier, Goetz
>>>> <[hidden email]>
>>>> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest
>>>> parsing of jar archives
>>>>
>>>>
>>>>> On 19 Jul 2018, at 11:46, Alan Bateman <[hidden email]>
>>>> 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.
Reply | Threaded
Open this post in threaded view
|

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

Baesken, Matthias

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

Hi Sean, Thanks for  the input  and comments .
Could we do the moving  of the  property parsing    do  in a followup CR, I would prefer this .

Best regards, Matthias


> -----Original Message-----
> From: Sean Mullan <[hidden email]>
> Sent: Freitag, 10. August 2018 17:39
> To: Baesken, Matthias <[hidden email]>; Chris Hegarty
> <[hidden email]>; Alan Bateman <[hidden email]>
> Cc: [hidden email]; security Dev OpenJDK <security-
> [hidden email]>
> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest
> parsing of jar archives
>
> 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' <[hidden email]>; Alan Bateman
> >>> <[hidden email]>
> >>> Cc: [hidden email]; Lindenmaier, Goetz
> >>> <[hidden email]>; Langer, Christoph
> >>> <[hidden email]>
> >>> 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 <[hidden email]>
> >>>> Sent: Donnerstag, 19. Juli 2018 14:54
> >>>> To: Alan Bateman <[hidden email]>; Baesken, Matthias
> >>>> <[hidden email]>
> >>>> Cc: [hidden email]; Lindenmaier, Goetz
> >>>> <[hidden email]>
> >>>> Subject: Re: [RFR] 8205525 : Improve exception messages during
> manifest
> >>>> parsing of jar archives
> >>>>
> >>>>
> >>>>> On 19 Jul 2018, at 11:46, Alan Bateman <[hidden email]>
> >>>> 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.
Reply | Threaded
Open this post in threaded view
|

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

Sean Mullan
On 8/13/18 11:18 AM, Baesken, Matthias wrote:
>> 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.
> Hi Sean, Thanks for  the input  and comments .
> Could we do the moving  of the  property parsing    do  in a followup CR, I would prefer this .

I think it should be done as part of this issue. It is too late to get
this into JDK 11, so I think it is better to take the time now to do the
code restructuring.

Thanks,
Sean
Reply | Threaded
Open this post in threaded view
|

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

Baesken, Matthias
In reply to this post by Sean Mullan
Hello, I created another webrev :

http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.5/

- use now  jarPath  instead of  jarpath  in the java security file
-  moved the parsing of the property  to a more general location (separate class jdk/internal/util/SecuritySystemPropertyHelper.java  )
-  read now  from the InputStream and check for error conditions *before*  setting jarFilename    in the Manifest  constructor        


Best regards , Matthias


> -----Original Message-----
> From: Sean Mullan <[hidden email]>
> Sent: Freitag, 10. August 2018 17:39
> To: Baesken, Matthias <[hidden email]>; Chris Hegarty
> <[hidden email]>; Alan Bateman <[hidden email]>
> Cc: [hidden email]; security Dev OpenJDK <security-
> [hidden email]>
> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest
> parsing of jar archives
>
> 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' <[hidden email]>; Alan Bateman
> >>> <[hidden email]>
> >>> Cc: [hidden email]; Lindenmaier, Goetz
> >>> <[hidden email]>; Langer, Christoph
> >>> <[hidden email]>
> >>> 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 <[hidden email]>
> >>>> Sent: Donnerstag, 19. Juli 2018 14:54
> >>>> To: Alan Bateman <[hidden email]>; Baesken, Matthias
> >>>> <[hidden email]>
> >>>> Cc: [hidden email]; Lindenmaier, Goetz
> >>>> <[hidden email]>
> >>>> Subject: Re: [RFR] 8205525 : Improve exception messages during
> manifest
> >>>> parsing of jar archives
> >>>>
> >>>>
> >>>>> On 19 Jul 2018, at 11:46, Alan Bateman <[hidden email]>
> >>>> 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.
Reply | Threaded
Open this post in threaded view
|

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

Alan Bateman
On 24/08/2018 15:52, Baesken, Matthias wrote:
> Hello, I created another webrev :
>
> http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.5/
>
> - use now  jarPath  instead of  jarpath  in the java security file
> -  moved the parsing of the property  to a more general location (separate class jdk/internal/util/SecuritySystemPropertyHelper.java  )
> -  read now  from the InputStream and check for error conditions *before*  setting jarFilename    in the Manifest  constructor
>
Will sun.net.util.SocketExceptions be changed to use the supporting
class or is that a separate issue?

In terms of approach then I think what you have is okay although I think
we should try to find a better name than "SecuritySystemPropertyHelper"
and also get feedback from security-dev on whether they would prefer it
in the sun.security tree with the other classes security classes.

-Alan
Reply | Threaded
Open this post in threaded view
|

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

Baesken, Matthias
Hi Alan ,

> Will sun.net.util.SocketExceptions be changed to use the supporting
> class or is that a separate issue?
>

I think this is a separate issue .

> In terms of approach then I think what you have is okay although I think
> we should try to find a better name than "SecuritySystemPropertyHelper"
> and also get feedback from security-dev on whether they would prefer it
> in the sun.security tree with the other classes security classes.

Suggestions are welcome , I  was a bit unsure about the name as well 😉 !

Thanks, Matthias


> -----Original Message-----
> From: Alan Bateman <[hidden email]>
> Sent: Montag, 27. August 2018 15:52
> To: Baesken, Matthias <[hidden email]>; Sean Mullan
> <[hidden email]>; Chris Hegarty <[hidden email]>
> Cc: [hidden email]; security Dev OpenJDK <security-
> [hidden email]>
> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest
> parsing of jar archives
>
> On 24/08/2018 15:52, Baesken, Matthias wrote:
> > Hello, I created another webrev :
> >
> > http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.5/
> >
> > - use now  jarPath  instead of  jarpath  in the java security file
> > -  moved the parsing of the property  to a more general location (separate
> class jdk/internal/util/SecuritySystemPropertyHelper.java  )
> > -  read now  from the InputStream and check for error conditions *before*
> setting jarFilename    in the Manifest  constructor
> >
> Will sun.net.util.SocketExceptions be changed to use the supporting
> class or is that a separate issue?
>
> In terms of approach then I think what you have is okay although I think
> we should try to find a better name than "SecuritySystemPropertyHelper"
> and also get feedback from security-dev on whether they would prefer it
> in the sun.security tree with the other classes security classes.
>
> -Alan
Reply | Threaded
Open this post in threaded view
|

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

Weijun Wang


> On Aug 27, 2018, at 10:25 PM, Baesken, Matthias <[hidden email]> wrote:
>
> Hi Alan ,
>
>> Will sun.net.util.SocketExceptions be changed to use the supporting
>> class or is that a separate issue?
>>
>
> I think this is a separate issue .
>
>> In terms of approach then I think what you have is okay although I think
>> we should try to find a better name than "SecuritySystemPropertyHelper"
>> and also get feedback from security-dev on whether they would prefer it
>> in the sun.security tree with the other classes security classes.
>
> Suggestions are welcome , I  was a bit unsure about the name as well 😉 !

I am OK with creating a sun.security.util.SecurityProperties class, and a method called privilegeGetOverridable() for the 1st part of SecuritySystemPropertyHelper::initTextProperty (by overridable I mean a system property can shadow the security property). I have no idea where the 2nd part should go. Maybe you can just create an includedInExceptions(String refName) method for this purpose.

So it will be something like this

public class SecurityProperties {

    public static String privilegeGetOverridable(String propName) {
        return AccessController.doPrivileged((PrivilegedAction<String>)
                () -> {
                    String val = System.getProperty(propName);
                    if (val == null) {
                        return Security.getProperty(propName);
                    } else {
                        return val;
                    }
                });
    }

    public static boolean includedInExceptions(String refName) {
        String val = privilegeGetOverridable("jdk.includeInExceptions");
        if (val == null){
            return false;
        }
        String[] tokens = val.split(",");
        for (String token : tokens) {
            if (token.equalsIgnoreCase(refName))
                return true;
        }
        return false;
    }
}

Thanks
Max


>
> Thanks, Matthias
>
>
>> -----Original Message-----
>> From: Alan Bateman <[hidden email]>
>> Sent: Montag, 27. August 2018 15:52
>> To: Baesken, Matthias <[hidden email]>; Sean Mullan
>> <[hidden email]>; Chris Hegarty <[hidden email]>
>> Cc: [hidden email]; security Dev OpenJDK <security-
>> [hidden email]>
>> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest
>> parsing of jar archives
>>
>> On 24/08/2018 15:52, Baesken, Matthias wrote:
>>> Hello, I created another webrev :
>>>
>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.5/
>>>
>>> - use now  jarPath  instead of  jarpath  in the java security file
>>> -  moved the parsing of the property  to a more general location (separate
>> class jdk/internal/util/SecuritySystemPropertyHelper.java  )
>>> -  read now  from the InputStream and check for error conditions *before*
>> setting jarFilename    in the Manifest  constructor
>>>
>> Will sun.net.util.SocketExceptions be changed to use the supporting
>> class or is that a separate issue?
>>
>> In terms of approach then I think what you have is okay although I think
>> we should try to find a better name than "SecuritySystemPropertyHelper"
>> and also get feedback from security-dev on whether they would prefer it
>> in the sun.security tree with the other classes security classes.
>>
>> -Alan

Reply | Threaded
Open this post in threaded view
|

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

Baesken, Matthias
Hi Max, thanks for your input .

I created another webrev , this contains now   the suggested  SecurityProperties   class :

http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.6/

Best regards, Matthias




> -----Original Message-----
> From: Weijun Wang <[hidden email]>
> Sent: Montag, 27. August 2018 17:35
> To: Baesken, Matthias <[hidden email]>
> Cc: Alan Bateman <[hidden email]>; Sean Mullan
> <[hidden email]>; Chris Hegarty <[hidden email]>;
> [hidden email]; [hidden email]
> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest
> parsing of jar archives
>
>
>
> > On Aug 27, 2018, at 10:25 PM, Baesken, Matthias
> <[hidden email]> wrote:
> >
> > Hi Alan ,
> >
> >> Will sun.net.util.SocketExceptions be changed to use the supporting
> >> class or is that a separate issue?
> >>
> >
> > I think this is a separate issue .
> >
> >> In terms of approach then I think what you have is okay although I think
> >> we should try to find a better name than "SecuritySystemPropertyHelper"
> >> and also get feedback from security-dev on whether they would prefer it
> >> in the sun.security tree with the other classes security classes.
> >
> > Suggestions are welcome , I  was a bit unsure about the name as well 😉 !
>
> I am OK with creating a sun.security.util.SecurityProperties class, and a
> method called privilegeGetOverridable() for the 1st part of
> SecuritySystemPropertyHelper::initTextProperty (by overridable I mean a
> system property can shadow the security property). I have no idea where
> the 2nd part should go. Maybe you can just create an
> includedInExceptions(String refName) method for this purpose.
>
> So it will be something like this
>
> public class SecurityProperties {
>
>     public static String privilegeGetOverridable(String propName) {
>         return AccessController.doPrivileged((PrivilegedAction<String>)
>                 () -> {
>                     String val = System.getProperty(propName);
>                     if (val == null) {
>                         return Security.getProperty(propName);
>                     } else {
>                         return val;
>                     }
>                 });
>     }
>
>     public static boolean includedInExceptions(String refName) {
>         String val = privilegeGetOverridable("jdk.includeInExceptions");
>         if (val == null){
>             return false;
>         }
>         String[] tokens = val.split(",");
>         for (String token : tokens) {
>             if (token.equalsIgnoreCase(refName))
>                 return true;
>         }
>         return false;
>     }
> }
>
> Thanks
> Max
>
>
> >
> > Thanks, Matthias
> >
> >
> >> -----Original Message-----
> >> From: Alan Bateman <[hidden email]>
> >> Sent: Montag, 27. August 2018 15:52
> >> To: Baesken, Matthias <[hidden email]>; Sean Mullan
> >> <[hidden email]>; Chris Hegarty <[hidden email]>
> >> Cc: [hidden email]; security Dev OpenJDK <security-
> >> [hidden email]>
> >> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest
> >> parsing of jar archives
> >>
> >> On 24/08/2018 15:52, Baesken, Matthias wrote:
> >>> Hello, I created another webrev :
> >>>
> >>> http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.5/
> >>>
> >>> - use now  jarPath  instead of  jarpath  in the java security file
> >>> -  moved the parsing of the property  to a more general location
> (separate
> >> class jdk/internal/util/SecuritySystemPropertyHelper.java  )
> >>> -  read now  from the InputStream and check for error conditions
> *before*
> >> setting jarFilename    in the Manifest  constructor
> >>>
> >> Will sun.net.util.SocketExceptions be changed to use the supporting
> >> class or is that a separate issue?
> >>
> >> In terms of approach then I think what you have is okay although I think
> >> we should try to find a better name than "SecuritySystemPropertyHelper"
> >> and also get feedback from security-dev on whether they would prefer it
> >> in the sun.security tree with the other classes security classes.
> >>
> >> -Alan

Reply | Threaded
Open this post in threaded view
|

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

Weijun Wang
SecurityProperties.java:

- Please add the copyright+license header to the new file.

- The "jdk.includeInExceptions" definition says there can be "Leading and trailing whitespaces". You might need to call trim() on each token.

java.security:

- I would suggest saying "classes from the java.util.jar package" or just "when parsing a jar file".

Others:

- What will the output look like? Is it "/tmp/x.jar:100"? If I read correctly, the line number is of the manifest and not the jar file.

Thanks
Max

> On Aug 29, 2018, at 10:01 PM, Baesken, Matthias <[hidden email]> wrote:
>
> Hi Max, thanks for your input .
>
> I created another webrev , this contains now   the suggested  SecurityProperties   class :
>
> http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.6/
>
> Best regards, Matthias
>
>
>
>
>> -----Original Message-----
>> From: Weijun Wang <[hidden email]>
>> Sent: Montag, 27. August 2018 17:35
>> To: Baesken, Matthias <[hidden email]>
>> Cc: Alan Bateman <[hidden email]>; Sean Mullan
>> <[hidden email]>; Chris Hegarty <[hidden email]>;
>> [hidden email]; [hidden email]
>> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest
>> parsing of jar archives
>>
>>
>>
>>> On Aug 27, 2018, at 10:25 PM, Baesken, Matthias
>> <[hidden email]> wrote:
>>>
>>> Hi Alan ,
>>>
>>>> Will sun.net.util.SocketExceptions be changed to use the supporting
>>>> class or is that a separate issue?
>>>>
>>>
>>> I think this is a separate issue .
>>>
>>>> In terms of approach then I think what you have is okay although I think
>>>> we should try to find a better name than "SecuritySystemPropertyHelper"
>>>> and also get feedback from security-dev on whether they would prefer it
>>>> in the sun.security tree with the other classes security classes.
>>>
>>> Suggestions are welcome , I  was a bit unsure about the name as well 😉 !
>>
>> I am OK with creating a sun.security.util.SecurityProperties class, and a
>> method called privilegeGetOverridable() for the 1st part of
>> SecuritySystemPropertyHelper::initTextProperty (by overridable I mean a
>> system property can shadow the security property). I have no idea where
>> the 2nd part should go. Maybe you can just create an
>> includedInExceptions(String refName) method for this purpose.
>>
>> So it will be something like this
>>
>> public class SecurityProperties {
>>
>>   public static String privilegeGetOverridable(String propName) {
>>       return AccessController.doPrivileged((PrivilegedAction<String>)
>>               () -> {
>>                   String val = System.getProperty(propName);
>>                   if (val == null) {
>>                       return Security.getProperty(propName);
>>                   } else {
>>                       return val;
>>                   }
>>               });
>>   }
>>
>>   public static boolean includedInExceptions(String refName) {
>>       String val = privilegeGetOverridable("jdk.includeInExceptions");
>>       if (val == null){
>>           return false;
>>       }
>>       String[] tokens = val.split(",");
>>       for (String token : tokens) {
>>           if (token.equalsIgnoreCase(refName))
>>               return true;
>>       }
>>       return false;
>>   }
>> }
>>
>> Thanks
>> Max
>>
>>
>>>
>>> Thanks, Matthias
>>>
>>>
>>>> -----Original Message-----
>>>> From: Alan Bateman <[hidden email]>
>>>> Sent: Montag, 27. August 2018 15:52
>>>> To: Baesken, Matthias <[hidden email]>; Sean Mullan
>>>> <[hidden email]>; Chris Hegarty <[hidden email]>
>>>> Cc: [hidden email]; security Dev OpenJDK <security-
>>>> [hidden email]>
>>>> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest
>>>> parsing of jar archives
>>>>
>>>> On 24/08/2018 15:52, Baesken, Matthias wrote:
>>>>> Hello, I created another webrev :
>>>>>
>>>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.5/
>>>>>
>>>>> - use now  jarPath  instead of  jarpath  in the java security file
>>>>> -  moved the parsing of the property  to a more general location
>> (separate
>>>> class jdk/internal/util/SecuritySystemPropertyHelper.java  )
>>>>> -  read now  from the InputStream and check for error conditions
>> *before*
>>>> setting jarFilename    in the Manifest  constructor
>>>>>
>>>> Will sun.net.util.SocketExceptions be changed to use the supporting
>>>> class or is that a separate issue?
>>>>
>>>> In terms of approach then I think what you have is okay although I think
>>>> we should try to find a better name than "SecuritySystemPropertyHelper"
>>>> and also get feedback from security-dev on whether they would prefer it
>>>> in the sun.security tree with the other classes security classes.
>>>>
>>>> -Alan
>

Reply | Threaded
Open this post in threaded view
|

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

Baesken, Matthias
Hi Max,

I changed the following in the new webrev :
- added the  copyright+license header to the new file
- called trim on each token

- adjusted the comment java.security  like you suggested

http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.7/

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


Best regards, Matthias



> -----Original Message-----
> From: Weijun Wang <[hidden email]>
> Sent: Mittwoch, 29. August 2018 16:27
> To: Baesken, Matthias <[hidden email]>
> Cc: Alan Bateman <[hidden email]>; Sean Mullan
> <[hidden email]>; Chris Hegarty <[hidden email]>;
> [hidden email]; [hidden email]
> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest
> parsing of jar archives
>
> SecurityProperties.java:
>
> - Please add the copyright+license header to the new file.
>
> - The "jdk.includeInExceptions" definition says there can be "Leading and
> trailing whitespaces". You might need to call trim() on each token.
>
> java.security:
>
> - I would suggest saying "classes from the java.util.jar package" or just "when
> parsing a jar file".
>
> Others:
>
> - What will the output look like? Is it "/tmp/x.jar:100"? If I read correctly, the
> line number is of the manifest and not the jar file.
>
> Thanks
> Max
>
> > On Aug 29, 2018, at 10:01 PM, Baesken, Matthias
> <[hidden email]> wrote:
> >
> > Hi Max, thanks for your input .
> >
> > I created another webrev , this contains now   the suggested
> SecurityProperties   class :
> >
> > http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.6/
> >
> > Best regards, Matthias
> >
> >
> >
> >
> >> -----Original Message-----
> >> From: Weijun Wang <[hidden email]>
> >> Sent: Montag, 27. August 2018 17:35
> >> To: Baesken, Matthias <[hidden email]>
> >> Cc: Alan Bateman <[hidden email]>; Sean Mullan
> >> <[hidden email]>; Chris Hegarty <[hidden email]>;
> >> [hidden email]; [hidden email]
> >> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest
> >> parsing of jar archives
> >>
> >>
> >>
> >>> On Aug 27, 2018, at 10:25 PM, Baesken, Matthias
> >> <[hidden email]> wrote:
> >>>
> >>> Hi Alan ,
> >>>
> >>>> Will sun.net.util.SocketExceptions be changed to use the supporting
> >>>> class or is that a separate issue?
> >>>>
> >>>
> >>> I think this is a separate issue .
> >>>
> >>>> In terms of approach then I think what you have is okay although I think
> >>>> we should try to find a better name than
> "SecuritySystemPropertyHelper"
> >>>> and also get feedback from security-dev on whether they would prefer
> it
> >>>> in the sun.security tree with the other classes security classes.
> >>>
> >>> Suggestions are welcome , I  was a bit unsure about the name as well 😉 !
> >>
> >> I am OK with creating a sun.security.util.SecurityProperties class, and a
> >> method called privilegeGetOverridable() for the 1st part of
> >> SecuritySystemPropertyHelper::initTextProperty (by overridable I mean a
> >> system property can shadow the security property). I have no idea where
> >> the 2nd part should go. Maybe you can just create an
> >> includedInExceptions(String refName) method for this purpose.
> >>
> >> So it will be something like this
> >>
> >> public class SecurityProperties {
> >>
> >>   public static String privilegeGetOverridable(String propName) {
> >>       return AccessController.doPrivileged((PrivilegedAction<String>)
> >>               () -> {
> >>                   String val = System.getProperty(propName);
> >>                   if (val == null) {
> >>                       return Security.getProperty(propName);
> >>                   } else {
> >>                       return val;
> >>                   }
> >>               });
> >>   }
> >>
> >>   public static boolean includedInExceptions(String refName) {
> >>       String val = privilegeGetOverridable("jdk.includeInExceptions");
> >>       if (val == null){
> >>           return false;
> >>       }
> >>       String[] tokens = val.split(",");
> >>       for (String token : tokens) {
> >>           if (token.equalsIgnoreCase(refName))
> >>               return true;
> >>       }
> >>       return false;
> >>   }
> >> }
> >>
> >> Thanks
> >> Max
> >>
> >>
> >>>
> >>> Thanks, Matthias
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Alan Bateman <[hidden email]>
> >>>> Sent: Montag, 27. August 2018 15:52
> >>>> To: Baesken, Matthias <[hidden email]>; Sean Mullan
> >>>> <[hidden email]>; Chris Hegarty
> <[hidden email]>
> >>>> Cc: [hidden email]; security Dev OpenJDK <security-
> >>>> [hidden email]>
> >>>> Subject: Re: [RFR] 8205525 : Improve exception messages during
> manifest
> >>>> parsing of jar archives
> >>>>
> >>>> On 24/08/2018 15:52, Baesken, Matthias wrote:
> >>>>> Hello, I created another webrev :
> >>>>>
> >>>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.5/
> >>>>>
> >>>>> - use now  jarPath  instead of  jarpath  in the java security file
> >>>>> -  moved the parsing of the property  to a more general location
> >> (separate
> >>>> class jdk/internal/util/SecuritySystemPropertyHelper.java  )
> >>>>> -  read now  from the InputStream and check for error conditions
> >> *before*
> >>>> setting jarFilename    in the Manifest  constructor
> >>>>>
> >>>> Will sun.net.util.SocketExceptions be changed to use the supporting
> >>>> class or is that a separate issue?
> >>>>
> >>>> In terms of approach then I think what you have is okay although I think
> >>>> we should try to find a better name than
> "SecuritySystemPropertyHelper"
> >>>> and also get feedback from security-dev on whether they would prefer
> it
> >>>> in the sun.security tree with the other classes security classes.
> >>>>
> >>>> -Alan
> >

Reply | Threaded
Open this post in threaded view
|

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

Weijun Wang


> On Aug 30, 2018, at 8:26 PM, Baesken, Matthias <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Baesken, Matthias


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/share/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;
         }
   .....


Best regards, Matthias



> -----Original Message-----
> From: Weijun Wang <[hidden email]>
> Sent: Donnerstag, 30. August 2018 16:04
> To: Baesken, Matthias <[hidden email]>
> Cc: Alan Bateman <[hidden email]>; Sean Mullan
> <[hidden email]>; Chris Hegarty <[hidden email]>;
> [hidden email]; [hidden email]
> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest
> parsing of jar archives
>
>
>
> > On Aug 30, 2018, at 8:26 PM, Baesken, Matthias
> <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Weijun Wang
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 <[hidden email]> 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/share/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 <[hidden email]>
>> Sent: Donnerstag, 30. August 2018 16:04
>> To: Baesken, Matthias <[hidden email]>
>> Cc: Alan Bateman <[hidden email]>; Sean Mullan
>> <[hidden email]>; Chris Hegarty <[hidden email]>;
>> [hidden email]; [hidden email]
>> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest
>> parsing of jar archives
>>
>>
>>
>>> On Aug 30, 2018, at 8:26 PM, Baesken, Matthias
>> <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

Weijun Wang
Or you can smuggle it out through JavaUtilJarAccess with SharedSecrets.

> On Aug 31, 2018, at 10:32 AM, Weijun Wang <[hidden email]> wrote:
>
> 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)?

Reply | Threaded
Open this post in threaded view
|

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

Baesken, Matthias
In reply to this post by Weijun Wang
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 { ... }

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


Best regards, Matthias



> -----Original Message-----
> From: Weijun Wang <[hidden email]>
> Sent: Freitag, 31. August 2018 04:32
> To: Baesken, Matthias <[hidden email]>
> Cc: Alan Bateman <[hidden email]>; Sean Mullan
> <[hidden email]>; Chris Hegarty <[hidden email]>;
> [hidden email]; [hidden email]
> 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
> <[hidden email]> 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 <[hidden email]>
> >> Sent: Donnerstag, 30. August 2018 16:04
> >> To: Baesken, Matthias <[hidden email]>
> >> Cc: Alan Bateman <[hidden email]>; Sean Mullan
> >> <[hidden email]>; Chris Hegarty <[hidden email]>;
> >> [hidden email]; [hidden email]
> >> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest
> >> parsing of jar archives
> >>
> >>
> >>
> >>> On Aug 30, 2018, at 8:26 PM, Baesken, Matthias
> >> <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

Weijun Wang


> On Aug 31, 2018, at 8:52 PM, Baesken, Matthias <[hidden email]> 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 <[hidden email]>
>> Sent: Freitag, 31. August 2018 04:32
>> To: Baesken, Matthias <[hidden email]>
>> Cc: Alan Bateman <[hidden email]>; Sean Mullan
>> <[hidden email]>; Chris Hegarty <[hidden email]>;
>> [hidden email]; [hidden email]
>> 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
>> <[hidden email]> 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 <[hidden email]>
>>>> Sent: Donnerstag, 30. August 2018 16:04
>>>> To: Baesken, Matthias <[hidden email]>
>>>> Cc: Alan Bateman <[hidden email]>; Sean Mullan
>>>> <[hidden email]>; Chris Hegarty <[hidden email]>;
>>>> [hidden email]; [hidden email]
>>>> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest
>>>> parsing of jar archives
>>>>
>>>>
>>>>
>>>>> On Aug 30, 2018, at 8:26 PM, Baesken, Matthias
>>>> <[hidden email]> 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
>

123