Re: Patch: rewrite File.toCanonicalPath() for GNU/Posix systems

[Gary Benson <gbenson at redhat dot com>]

Tom Tromey wrote:
> >>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:
> 
> Gary> The diff of the method itself is not particularly legible,
> Gary> so I've attached a copy of the new method as well.
> 
> This is looking good.
> 
> I had a couple of questions though.
> 
> First:
> 
> -  // Special case: treat "" the same as ".".
> -  if (total == 0)
> -    buf[total++] = '.';
> 
> Is this now handled some other way?  I didn't try to trace through
> the logic.  I forget if there is a test for this... if not there
> ought to be.

The very first call in the method ensures the path is absolute:

+  jstring path = getAbsolutePath ();

So the smallest possible path is "/".  The previous implementation did
not ensure paths were absolute, which was an error.

> Gary>       // Unlike other JVMs we do not rewind past the root
> Gary>       // directory.  I can't see any legitimate reason why you
> Gary>       // would want this, and chopping off bits of path seems
> Gary>       // like a sure-fire way to introduce vulnerabilities.
> 
> I'm curious about this.  It seems a bit weird that it would be valid
> to try to open "../../../foo" but not to get the canonical path name
> of that same file.

It'll give you a canonical path for "../../../foo" so long as your
current directory has at least three elements in it.  It's paths like
"/../foo" that will cause the exception to be thrown.

I added this check because I managed to confuse a JVM into not
resolving symlinks this way:

  File.getCanonicalPath("/bin/view") => "/bin/vi"
  File.getCanonicalPath("/home/../bin/view") => "/bin/vi"
  File.getCanonicalPath("/../bin/view") => "/bin/view"
                                                 ^^^^

I originally thought this couldn't be avoided if you allowed rewinding
past root, but some later changes I made mean my implementation will
cope with this just fine, so the check could be removed.  It all
depends whether you think "/../foo" is a valid path... personally I'm
not sure either way.

Cheers,
Gary