Re: RFR: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause

2022-05-11 Thread Sean Mullan
On Wed, 11 May 2022 05:39:42 GMT, Alan Bateman  wrote:

> > > It's probably ok, but the bug report is either incomplete or I am missing 
> > > something. It says "This can be improved to something like: ..." but the 
> > > same text as is emitted now is used. Can you fix this so I have a better 
> > > example of what will be included in the message?
> > 
> > 
> > The bug report also says "The error message does not give a clue which jar 
> > file is causing the problem" but the error message includes the name 
> > "invalid.jar" so I am also confused about that.
> 
> There are two parts to it. In the case of initCEN method, the proposed change 
> is to include the name of the rejected entry in the exception message. This 
> is not the same thing as leaking a file path in the exception message so I 
> don't think we have a concern here.

Ok, but @RealCLanger can you address the prior comments I had on the bug 
report? The error messages (before and after the fix) are the same.

-

PR: https://git.openjdk.java.net/jdk/pull/8616


Re: RFR: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause

2022-05-10 Thread Alan Bateman
On Tue, 10 May 2022 21:30:23 GMT, Sean Mullan  wrote:

> > It's probably ok, but the bug report is either incomplete or I am missing 
> > something. It says "This can be improved to something like: ..." but the 
> > same text as is emitted now is used. Can you fix this so I have a better 
> > example of what will be included in the message?
> 
> The bug report also says "The error message does not give a clue which jar 
> file is causing the problem" but the error message includes the name 
> "invalid.jar" so I am also confused about that.

There are two parts to it. In the case of initCEN method, the proposed change 
is to include the name of the rejected entry in the exception message. This is 
not the same thing as leaking a file path in the exception message so I don't 
think we have a concern here.

-

PR: https://git.openjdk.java.net/jdk/pull/8616


Re: RFR: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause

2022-05-10 Thread Sean Mullan
On Tue, 10 May 2022 21:26:42 GMT, Sean Mullan  wrote:

> It's probably ok, but the bug report is either incomplete or I am missing 
> something. It says "This can be improved to something like: ..." but the same 
> text as is emitted now is used. Can you fix this so I have a better example 
> of what will be included in the message?

The bug report also says "The error message does not give a clue which jar file 
is causing the problem" but the error message includes the name "invalid.jar" 
so I am also confused about that.

-

PR: https://git.openjdk.java.net/jdk/pull/8616


Re: RFR: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause

2022-05-10 Thread Sean Mullan
On Tue, 10 May 2022 16:59:41 GMT, Lance Andersen  wrote:

> > > > > @LanceAndersen @AlanBateman do you think adding the entry name in the 
> > > > > exception in ZipFileSystem is ok? If so, should it maybe go into a 
> > > > > different patch?
> > > > 
> > > > 
> > > > It should be okay as this is the name of an entry in the zip file. It 
> > > > might be a bit cleaner to add a method to IndexNode to return the name 
> > > > as String. Alternatively maybe its toString could be changed to drop 
> > > > the index (I would need to dig into the history to find out if there is 
> > > > really any use for the index in the String representation).
> > > 
> > > 
> > > I think this would be OK, but would get to get someone from our security 
> > > team to bless it.
> > > It might not be a bad idea to add a method to return the name as a 
> > > String. There are a couple of places where we do a new String(name) so 
> > > would economize any future changes
> > 
> > 
> > Sounds fair. @LanceAndersen, can you please ask the security team about 
> > their ok then and let me know? In case their answer is a yes, I'll work on 
> > implementing the suggestion to return the name as String. Shall I maybe do 
> > the zipfs change in a different PR then? The more important change in the 
> > context of javac is printing out the jar name in javac itself.
> 
> Already did ;-) so hopefully they will share their thoughts soon.

It's probably ok, but the bug report is either incomplete or I am missing 
something. It says "This can be improved to something like: ..." but the same 
text as is emitted now is used. Can you fix this so I have a better example of 
what will be included in the message?

-

PR: https://git.openjdk.java.net/jdk/pull/8616


Re: RFR: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause

2022-05-10 Thread Lance Andersen
On Tue, 10 May 2022 16:58:03 GMT, Lance Andersen  wrote:

>>> I think this would be OK, but would get to get someone from our security 
>>> team to bless it.
>> 
>> It's print the entry name, I don't think it is leaking the file path to the 
>> zip file.
>
>> > I think this would be OK, but would get to get someone from our security 
>> > team to bless it.
>> 
>> It's print the entry name, I don't think it is leaking the file path to the 
>> zip file.
> 
> I think you are probably right I am probably being overly cautious

> > > > > @LanceAndersen @AlanBateman do you think adding the entry name in the 
> > > > > exception in ZipFileSystem is ok? If so, should it maybe go into a 
> > > > > different patch?
> > > > 
> > > > 
> > > > It should be okay as this is the name of an entry in the zip file. It 
> > > > might be a bit cleaner to add a method to IndexNode to return the name 
> > > > as String. Alternatively maybe its toString could be changed to drop 
> > > > the index (I would need to dig into the history to find out if there is 
> > > > really any use for the index in the String representation).
> > > 
> > > 
> > > I think this would be OK, but would get to get someone from our security 
> > > team to bless it.
> > > It might not be a bad idea to add a method to return the name as a 
> > > String. There are a couple of places where we do a new String(name) so 
> > > would economize any future changes
> > 
> > 
> > Sounds fair. @LanceAndersen, can you please ask the security team about 
> > their ok then and let me know? In case their answer is a yes, I'll work on 
> > implementing the suggestion to return the name as String. Shall I maybe do 
> > the zipfs change in a different PR then? The more important change in the 
> > context of javac is printing out the jar name in javac itself.
> 
> Already did ;-) so hopefully they will share their thoughts soon.

I think it would probably be good for a separate PR for the ZipFS change as it 
keeps it a bit clearer

-

PR: https://git.openjdk.java.net/jdk/pull/8616


Re: RFR: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause

2022-05-10 Thread Lance Andersen
On Tue, 10 May 2022 16:51:35 GMT, Alan Bateman  wrote:

> > I think this would be OK, but would get to get someone from our security 
> > team to bless it.
> 
> It's print the entry name, I don't think it is leaking the file path to the 
> zip file.

I think you are probably right I am probably being overly cautious

-

PR: https://git.openjdk.java.net/jdk/pull/8616


Re: RFR: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause

2022-05-10 Thread Lance Andersen
On Tue, 10 May 2022 16:30:01 GMT, Lance Andersen  wrote:

>>> @LanceAndersen @AlanBateman do you think adding the entry name in the 
>>> exception in ZipFileSystem is ok? If so, should it maybe go into a 
>>> different patch?
>> 
>> It should be okay as this is the name of an entry in the zip file. It might 
>> be a bit cleaner to add a method to IndexNode to return the name as String. 
>> Alternatively maybe its toString could be changed to drop the index (I would 
>> need to dig into the history to find out if there is really any use for the 
>> index in the String representation).
>
>> > @LanceAndersen @AlanBateman do you think adding the entry name in the 
>> > exception in ZipFileSystem is ok? If so, should it maybe go into a 
>> > different patch?
>> 
>> It should be okay as this is the name of an entry in the zip file. It might 
>> be a bit cleaner to add a method to IndexNode to return the name as String. 
>> Alternatively maybe its toString could be changed to drop the index (I would 
>> need to dig into the history to find out if there is really any use for the 
>> index in the String representation).
> 
> I think this would be OK, but would get to get someone from our security team 
> to bless it.  
> 
> It might not be a bad idea to add a method to return the name as a String.  
> There are a couple of places where we do a new String(name) so would 
> economize any future changes

> > > > @LanceAndersen @AlanBateman do you think adding the entry name in the 
> > > > exception in ZipFileSystem is ok? If so, should it maybe go into a 
> > > > different patch?
> > > 
> > > 
> > > It should be okay as this is the name of an entry in the zip file. It 
> > > might be a bit cleaner to add a method to IndexNode to return the name as 
> > > String. Alternatively maybe its toString could be changed to drop the 
> > > index (I would need to dig into the history to find out if there is 
> > > really any use for the index in the String representation).
> > 
> > 
> > I think this would be OK, but would get to get someone from our security 
> > team to bless it.
> > It might not be a bad idea to add a method to return the name as a String. 
> > There are a couple of places where we do a new String(name) so would 
> > economize any future changes
> 
> Sounds fair. @LanceAndersen, can you please ask the security team about their 
> ok then and let me know? In case their answer is a yes, I'll work on 
> implementing the suggestion to return the name as String. Shall I maybe do 
> the zipfs change in a different PR then? The more important change in the 
> context of javac is printing out the jar name in javac itself.

Already did ;-)   so hopefully they will share their thoughts soon.

-

PR: https://git.openjdk.java.net/jdk/pull/8616


Re: RFR: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause

2022-05-10 Thread Alan Bateman
On Tue, 10 May 2022 16:48:30 GMT, Christoph Langer  wrote:

> I think this would be OK, but would get to get someone from our security team 
> to bless it.

It's print the entry name, I don't think it is leaking the file path to the zip 
file.

-

PR: https://git.openjdk.java.net/jdk/pull/8616


Re: RFR: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause

2022-05-10 Thread Christoph Langer
On Tue, 10 May 2022 16:30:01 GMT, Lance Andersen  wrote:

> > > @LanceAndersen @AlanBateman do you think adding the entry name in the 
> > > exception in ZipFileSystem is ok? If so, should it maybe go into a 
> > > different patch?
> > 
> > 
> > It should be okay as this is the name of an entry in the zip file. It might 
> > be a bit cleaner to add a method to IndexNode to return the name as String. 
> > Alternatively maybe its toString could be changed to drop the index (I 
> > would need to dig into the history to find out if there is really any use 
> > for the index in the String representation).
> 
> I think this would be OK, but would get to get someone from our security team 
> to bless it.
> 
> It might not be a bad idea to add a method to return the name as a String. 
> There are a couple of places where we do a new String(name) so would 
> economize any future changes

Sounds fair. @LanceAndersen, can you please ask the security team about their 
ok then and let me know? In case their answer is a yes, I'll work on 
implementing the suggestion to return the name as String. Shall I maybe do the 
zipfs change in a different PR then? The more important change in the context 
of javac is printing out the jar name in javac itself.

-

PR: https://git.openjdk.java.net/jdk/pull/8616


Re: RFR: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause

2022-05-10 Thread Lance Andersen
On Tue, 10 May 2022 14:02:14 GMT, Alan Bateman  wrote:

> > @LanceAndersen @AlanBateman do you think adding the entry name in the 
> > exception in ZipFileSystem is ok? If so, should it maybe go into a 
> > different patch?
> 
> It should be okay as this is the name of an entry in the zip file. It might 
> be a bit cleaner to add a method to IndexNode to return the name as String. 
> Alternatively maybe its toString could be changed to drop the index (I would 
> need to dig into the history to find out if there is really any use for the 
> index in the String representation).

I think this would be OK, but would get to get someone from our security team 
to bless it.  

It might not be a bad idea to add a method to return the name as a String.  
There are a couple of places where we do a new String(name) so would economize 
any future changes

-

PR: https://git.openjdk.java.net/jdk/pull/8616


Re: RFR: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause

2022-05-10 Thread Alan Bateman
On Mon, 9 May 2022 22:32:57 GMT, Christoph Langer  wrote:

> @LanceAndersen @AlanBateman do you think adding the entry name in the 
> exception in ZipFileSystem is ok? If so, should it maybe go into a different 
> patch?

It should be okay as this is the name of an entry in the zip file. It might be 
a bit cleaner to add a method to IndexNode to return the name as String. 
Alternatively maybe its toString could be changed to drop the index (I would 
need to dig into the history to find out if there is really any use for that in 
the String representation).

-

PR: https://git.openjdk.java.net/jdk/pull/8616


Re: RFR: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause

2022-05-10 Thread Martin Doerr
On Mon, 9 May 2022 22:10:54 GMT, Christoph Langer  wrote:

> After https://bugs.openjdk.java.net/browse/JDK-8251329, javac throws errors 
> when the classpath
> contains jar files with . or .. in its name. The error message, however, does 
> not help to find
> the culprit. This could be improved.

LGTM. I think this is an important improvement. Developers should not be left 
alone figuring out which .jar file is responsible for the problem. There is 
currently no hint at all without this change.
(Note: Pre-submit test issues on 32 bit are unrelated.)

-

Marked as reviewed by mdoerr (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8616


Re: RFR: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause

2022-05-09 Thread Christoph Langer
On Mon, 9 May 2022 22:10:54 GMT, Christoph Langer  wrote:

> After https://bugs.openjdk.java.net/browse/JDK-8251329, javac throws errors 
> when the classpath
> contains jar files with . or .. in its name. The error message, however, does 
> not help to find
> the culprit. This could be improved.

@LanceAndersen @AlanBateman do you think adding the entry name in the exception 
in ZipFileSystem is ok? If so, should it maybe go into a different patch?

-

PR: https://git.openjdk.java.net/jdk/pull/8616