[Patch] 6502392 Invalid relative names for Filer.createResource and Filer.getResource
Jonathan Gibbons
Jonathan.Gibbons at Sun.COM
Mon Jan 19 18:46:40 PST 2009
IllegalArgumentException would seem a better choice than
AssertionError, since the only "assrtion" being tested is that the
caller has provided a valid argument.
The method is only static because it can be, and because it was
natural when it was written. We could make it not static if that bwere
convenient, but using log seems wrong -- that is for reporting user-
level diagnostics.
As a very minor point, note that the coding convention tends towards a
space between 'if' and '('. More pragmatically, the rule would be
something like "as much as possible, make your code look like the
surrounding code".
Happy travels.
-- Jon
On Jan 19, 2009, at 6:02 PM, Emil Siemes wrote:
> Hi Jon,
>
> sorry for the radio silence. I was and still am travelling. Your
> comments makes perfectly sense to me. I didn't dare to ask for
> changing the signature of isRelativeURI ;-)
> I would also like to suggest to rename the method to isRelativePath.
> Passing the path as String gives also the chance to check if we hit
> one of those rare cases where we misinterpret the path like
> foo:bar.xml.
>
> protected static boolean isRelativeUri(String relativeName) {
> URI uri = URI.create(relativeName);
> String path= uri.getPath();
>
> if(!relativeName.equals(uri.getRawPath())) {
> throw new AssertionError("Unsupported relative name:
> " + relativeName);
> }
>
> if(path== null || path.length()== 0 || path.charAt(0)== '/') {
> return false;
> }
>
> if(!uri.normalize().getPath().equals(relativeName))
> return false;
>
> return true;
> }
>
> This implementation would throw an AsserionError for those unusal
> paths. Is that the correct handling? Or should I better just log a
> warning (The method is static and doesn't have automatic access to
> the log object)?
> The path==null check is also only necessary for "odd" urls where
> URI.getPath() does indeed return null in some cases. So if we throw
> an exception we can delete it but if we log a warning only I would
> prefer to keep it to prevent any NPEs.
>
> Best Regards
> Emil
>
>
>
> Von: Jonathan Gibbons <Jonathan.Gibbons at Sun.COM>
> An: Emil Siemes <digitalemil at yahoo.de>
> CC: compiler-dev at openjdk.java.net
> Gesendet: Dienstag, den 13. Januar 2009, 01:37:04 Uhr
> Betreff: Re: AW: AW: AW: [Patch] 6502392 Invalid relative names for
> Filer.createResource and Filer.getResource
>
> Emil,
>
> I think the way forward is to go back and look at the spec of Filer,
> and to some extent, what was likely the
> original intent of the spec.
>
> Here's the relevant part of the File spec, from http://java.sun.com/javase/6/docs/api/javax/annotation/processing/Filer.html
>
>> The methods for reading and writing resources take a relative name
>> argument. 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.
>
> The primary definition is "non-null, non-empty sequence of path
> segments separated by '/'". It goes on to say that . and .. are
> invalid. Finally, it says they must match a spec in a section of rfc
> 3986. In particular, the Filer spec does not say that all
> productions of the path-rootless rule are valid arguments.
>
> So, to the intent. I think that the intent is to (only) allow
> lowest common denominator of relative filenames across Java
> platforms. As such, allowing ":" seems a very problematic character,
> given its significance on Windows.
>
> So, to your patch. This is definitely heading in the right
> direction, although as written isRelativeURI does not check for
> port, authority, query and fragment. [It never did.] So, I have one
> suggestion. If you read the javadoc comments for isRelativeFile,
> and follow the references, you get to this part of the spec athttp://java.sun.com/javase/6/docs/api/javax/tools/JavaFileManager.html#relative_name
> This uses the same words as found on Filer, with the following
> interesting addition:
>> Informally, this should be true:
>> URI.create(relativeName).normalize().getPath().equals(relativeName)
> This expression is very similar to the expression you came up
> with. However, it uses "String relativeName", rather than a URI.
> Looking at JavacFileManager, isRelativeURI is only called 3 times,
> and always on the result of URI.create. Since it is a protected
> static method on a javac-internal API, it would be reasonable to
> change the signature to take a String instead of a URI, and then we
> could formally check the informal assertion given above.
>
> For example, the code might look something like:
>
> protected static boolean isRelativeUri(String relativeName) {
> URI uri = URI.create(relativeName);
> String path= uri.getPath();
>
> if(path.length()> 0 && path.charAt(0)== '/') {
> return false;
> }
>
> if(path.length()== 0)
> return false;
>
> if(!uri.normalize().getPath().equals(relativeName))
> return false;
>
> return true;
> }
>
> In terms of cleanup, this would also allow us to remove the three
> comments // FIXME 6419701 and merge them into a single new comment
> on the URI.create line in isRelativeUri.
>
> You can also simplify the code a tiny bit more by reordering the
> first two if statements and thereby eliminating the redundant check
> on path.length() > 0.
>
>
> -- Jon
>
>
>
> Emil Siemes wrote:
>>
>> 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/20090119/b4a3e2b5/attachment.html
More information about the compiler-dev
mailing list