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

[Christoph Langer]

On Tue, 10 May 2022 16:59:41 GMT, Lance Andersen <lancea at openjdk.org> 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
>
>> > > > > @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

> > > > > > @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?

Good catch, I pasted two times the error message after the proposed patch. Fixed.

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

I've factored out the zipfs change into #8655. So this change only affects javac now.

-------------

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