AW: AW: AW: [Patch] 6502392 Invalid relative names for Filer.createResource and Filer.getResource

[Emil Siemes]

Hi Jon,

thank you very much for providing insight!

Please find an improved isRelativeUri method below. It fixes accepting invalid paths like foo/../bar and accepts valid path like .something
Unfortunately I now believe that using URI is not the right way to go:
"foo:/bar.xml" is a valid relative pathname but when we construct a URI("foo:/bar.xml") the first part "foo:" is treated as schema obviously.
getPath() then returns /bar.xml and this fails isRelative.
Similar new URI("foo#bar.xml").getPath() returns "foo" and is accepted by isRelative while "foo#bar.xml" is a non-valid relative path.

I tried using other URI constructors with explicitly setting schema and fragment to "" but URI doesn't accept this: 
 java.net.URISyntaxException: Relative path in absolute URI: ://src/foo.java#
It looks to me like using URI in our context is somehow miss-using the class and it will be difficult to make it water-proof.


I think we can either use URI in isRelative and accept that there are still glitches or create a isRelative which parses the path on its own.
Please let me know what you think. If you prefer a URI based isRelative I would create the patch with the method below. If you prefer a URI-free version now I will create a new patch since the path I sent out before also uses URI.


protected static boolean isRelativeUri(URI uri) {
        String path= uri.getPath();

        if(path.length()> 0 && path.charAt(0)== '/') {
            return false;
        }

        if(path.length()== 0)
            return false;
        
        if(!path.equals(uri.normalize()..getPath()))
                return false;
        
        return true;
      }








________________________________
Von: Jonathan Gibbons <Jonathan.Gibbons at Sun.COM>
An: Emil Siemes <digitalemil at yahoo.de>
CC: compiler-dev at openjdk.java.net
Gesendet: Freitag, den 9. Januar 2009, 00:33:37 Uhr
Betreff: Re: AW: AW: [Patch] 6502392 Invalid relative names for Filer.createResource and Filer.getResource

Emil,

Comments inline.

-- Jon


Emil Siemes wrote: 
Hi Jon,

RFC 3986 Appendix D describes the additions and modifications
vs. 2396..
Basically 3986 tries to make things clearer. They say there have
been a lot of discussions and different interpretations of 2396.
(Sounds like JME fragmentation to me ;-)
A new ABNF was used, IPV6 literals added. URI normalization and
comparison was rewritten.
Clarification of reserved and unreserved characters. Support for
an empty path component were added.
OK, thanks for the summary.


6791041 & 6791060 seem to be non-public. Will this change
soon? I were not able to find them on bugs.sun.com
They should now be available on bugs.sun.com.  There is sometimes a
short delay in propogating bugs 
from the internal bug database to the external site.



If there is more information available on 6791041 I could look
for potential interpretation issues in the java.net.URI implementation
vs. 3986 spec.
6791041 provided a case where someone actually tried to have a relative
path beginning with ".", such as ".foo"



I should have a new isRelativeURI Monday latest. Would be good
to know what 6791041 is about by then.
Apparently, there was an attempt in JDK 6 to update URI for 3986, and
the work had to be reverted
for compatibility issues. This makes me less optimistic that we might
see any evolution for JDK 7.
As such, I think we should leverage what is in URI as much as possible,
and apply any necessary
extra work in JavacFileManager -- which is essentially where you are
heading, I believe.




For the missing FileNotFoundException in JavacFiler I think the
clearest solution is to explicitly check for file existence in the
method. 
Something like:

getResource(...

FileObject fileobject= ...

if(!new File(fileobject.toURI().exists())
     throw new FileNotFoundException();
}

Unfortunately this would include another creation of an URI
object but FileObject..getName() only returns a "user friendly" file
name which I understand is not necessarily useable for a File-Object
creation.
The fileobject involved in getResource & createResource is
actually a com.sun.tools.javac.file.RegularFileObject with a File
object as field. So another solution would be to either expose this
File object or add an exists() method to RegularFileObject which
delegates the call to file Object and cast the FileObject to a
RegularFileObject. (too ugly I believe).
You cannot call "new File(...)" or more generally, use any of the File
API outside of the javac.file package, except to some extent in the
command line processing classes (Main and JavaCompiler.) This is
because the compilation units might not be coming directly from files
in the host file system.   In an IDE using JSR199, for example, the
compilation units might come from a database or from string buffers,
depending on the implementation of JavaFileManager in use. The only
real solution would be to add a new exists() method to FileObject and
use that, but that too would be problematic because that would be a
binary-incompatible change. The best we can probably do in the short
term is to test if the
fileObject is an instance of RegularFileObject or perhaps
BaseFileObject, and only apply the check in that case. Which is
somewhat uglier than your "too ugly I believe" but arguably the best we
can do, for now. :-(

Longer term, I note as background that JavaFileManager was designed as
a "virtual file system as defined by the needs of javac". Some wise
person once made comments about "as simple as possible but no
simpler".  I think JavaFileManager ended up a tad too simple. 
Separately, JSR 203 ("Son of NIO") is providing a more general virtual
file system API.  I think it would be interesting to see an impl of
JavaFileManager/FileObject/JavaFileObject built on top of the JSR 203
APIs. This would then allow applications that wanted to use JSR199 to
drive the compiler using a "non-standard" file system to use the JSR
203 API to describe that file system instead of the JavaFileManager
API.  Put another way, if JSR203 had been available in time for JDK 6,
I don't think we would have bothered with JavaFileManager etc.  The
point of all of which is to say that the way forward here is to
(eventually) provide an adapter for JSR 203, and to encourage folk to
use JSR 203 as a way to provide virtual file systems.




Thanks
   Emil












________________________________
Von: Jonathan Gibbons <Jonathan.Gibbons at Sun.COM>
An: Emil Siemes <digitalemil at yahoo.de>
CC: compiler-dev at openjdk.java.net
Gesendet: Mittwoch,
den 7. Januar 2009, 18:07:35 Uhr
Betreff: Re: AW:
[Patch] 6502392 Invalid relative names for Filer.createResource and
Filer.getResource

Emil, 

First, some updates.
1) 6791041 has come in, which reports a specific instance of a
user encountering the issue you have been looking at. Specifically,
the submitter is affected by the "quick and dirty" check in
isRelativeURI.
2) I have filed 6791060 against java.net.URI

It sounds like you are reasonably familiar with the RFCs
involved here. If so, can you briefly summarize any relevant differences
between RFC 2396 (used by java.net.URI) and RFC 3986 (used by
JSR 269 and Filer.)

-- Jon


On Jan 6, 2009, at 8:21 AM, Emil Siemes wrote:

Hi Jon,

true, the code actually does things which
would belong into java.net.URI. Reason for this is that JSR269 requires
compliance with path-rootless rule of RFC3986 whereas java.net.URI
supports the older and obsolete RFC 2396. Basically I was afraid of
relying on the old implementation but that can be changed of course.
Given the limitation of the actual URI
implementation it might be tricky to achieve compliance with the JSR269
requirements without parsing URI..getPath(). Dot-segments are not
allowed as relative path names in JSR269 but are fine for the URI
class. The current implementation calls URI.normalize().getPath() which
actually removes dot-segments and therefore doesn't throw an exception. 

Given your feedback I propose I rewrite
my code in a way that it relies on java.net.URI and does a quick check
on the path. May be I can compare URI.getPath() and
URI.normalize().getPath() to find out if any dot-segments are part of
the path. I'll check and let you know.

Thanks
   Emil


 




________________________________
Von: Jonathan Gibbons <Jonathan.Gibbons at Sun.COM>
An: Emil Andreas Siemes <digitalemil at yahoo.de>
CC: compiler-dev at openjdk.java.net
Gesendet: Dienstag, den 6. Januar 2009,
02:11:22 Uhr
Betreff: Re: [Patch] 6502392 Invalid
relative names for Filer.createResource and Filer.getResource

Emil,

Thank you for looking at this, and for your patch.

I do not understand why 6419701 is not public. I am trying to find out
why it is not.
The bug itself is about the use of URI.create, not about the use of
isRelativeURI..

I'm not sure that your patch is warranted (or arguably, the correct
fix).  Your patch certainly 
contains functionality that on the face of it would better belong in
java.net.URI and not 
JavacFileManager. 

The way I read the code in JavacFileManager, isRelativeURI is not
intended to be a full and 
complete implementation of checking whether a URI is path-rootless.
Instead, it is used in 
exactly 3 places, and always in conjunction with URI.create. 
JavacFileManager is assuming 
the full syntax checking is done by URI.create, and is just using a
simple impl of 
isRelativeURI to distinguish relative URIs from absolute URIs created
by URI.create.

6419701 is about removing the use of URI.create -- presumably because
it is mildly icky
to create the URI object just to check whether a path string is a valid
URI.  So it seems 
to me that there are two ways to take this issue:  
1) Rewrite isRelativeURI to take a String argument and thereby avoid
the use of URI.create
2) Wait for URI to have some better method for checking the validity of
a path without
actually constructing the URI object.

I guess that I dislike JavacFileManager having code for detailed
parsing of URIs, and so
I think the current impl is better, of using URI.create to check the
syntax of a URI, and
then apply a quick check as to whether it is absolute or relative is
better.

Or am I misreading the situation or perhaps missing some subtlety in
your proposed code?

-- Jon


Emil Andreas Siemes wrote: 
com.sun.tools.javac.file.JavacFileManager
checks if a name fulfills the requirements of relative names:

"A relative name is a
non-null, non-empty sequence of path segments separated by '/'; '.' and '..' are
invalid path segments. A valid relative name must match the
"path-rootless" rule of RFC 3986,
section 3.3."

This is done via the isRelativeUri()
method. Unfortunately this method doesn't currently really check for
path-rootless compliance. A patch with an improved method is attached.
When java.net.Uri eventually supports
RFC 3986 the need for isRelativeUri() goes away.. I would also like to
propose to rename isRelativeUri() to isRF3986PathRootless() which would
more clearly describe what the method does.

There is a comment in JavacFileManager
in a couple of lines:
//FIX ME 6419701
but I don't find bug 6419701 in the bug
database. Any thoughts?

Thanks
    Emil




 
________________________________


________________________________


      
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20090111/c800bf38/attachment.html