Bug 194418

Created attachment 105089 [details]
Test to enforce consistency of ".."

Nobody expected that ".." could be used in argument of getFileObject! The base filesystems in openide.filesystem module don't support this value at all. The masterfs can however handle it.

We need a consistency here. I can either restrict use of ".." in masterfs or implement support for ".." in all filesystems. I am in favour of the first solution.

You shall use fo.getParent() when you want to get filesystem parent.

I'm fine if you can restrict of ".." in masterfs, but to be consistent with that you should restrict use of '..' in findResource for masterfs as well, right?

ergonomics#4a6cfcd35eb7

Jarda,
what about FileUtil.toFileObject?
In "release" mode will be no conversion to normalized path => semantic was changed by returning "null" while before it returned FO (with incorrect path)

You are supposed to call FileUtil.toFileObject only on normalized files. There is even an assert to verify that. Calling FileUtil.toFileObject on non-normalized files is a bug. I am not sure what I shall fix.

Integrated into 'main-golden', will be available in build *201101190000* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/4a6cfcd35eb7
User: Jaroslav Tulach <jtulach@netbeans.org>
Log: #194418: Relative path cannot contain '..' anymore

(In reply to comment #5)
> You are supposed to call FileUtil.toFileObject only on normalized files. There
> is even an assert to verify that. Calling FileUtil.toFileObject on
> non-normalized files is a bug. I am not sure what I shall fix.
This assert unfortunately works only in "debug" mode (-ea) and doesn't work in -da mode. In code you can see that in -ea mode input file is changed to normalized version always.
But it is not the case for -da mode
So, my remark is about different runtime behavior of API in -ea and -da modes which is incorrect from API point of view

i.e. you can add 
        // return null for UNC root and paths with ".."
        if(file.getPath().equals("\\\\") || file.getPath().contains("..") {
            return null;
        }

I let Jesse to address the concern about use of assert in FileUtil.toFileObject. It is his API creation, as far as I know.

Just a note: I believe it's not about the assertion, 
but rather about assigning normalized file in FileUtil.java:1017:
    file = normFile;
this makes behavior differ in -ea and -da modes

(In reply to comment #10)
>     file = normFile;
> this makes behavior differ in -ea and -da modes

True; if code gets to this point the infrastructure is just trying to recover with minimal damage so other IDE functionality can continue to work in dev builds. In -da mode, it is simply too expensive to check for this condition. The calling code should be fixed to make sure this is a moot point.

The solution you choose looks to be incompatible API change (at least in terms of cluelessness). Something worked ok for developers and now their code is broken in runtime in a way which is quite hard to trace. The javadoc says nothing about .. in relative path param. At least the javadoc should be updated as well.

I think just document that ../ segments are forbidden in the relativePath argument. As far as I know there was never any intention to support such a syntax.

Even there was no intention it may be used somewhere already. 
At least it's he case for me, on Jan 18 morning some weblogic code works ok but on Jan 19 all was broken. It's good it's new functionality and was discovered immediatly and also may be updated now but what if there are more places dependent on '..' usage?

> We need a consistency here. I can either restrict use of ".." in masterfs or
> implement support for ".." in all filesystems. I am in favour of the first
> solution.
Jarda, looks like the first approach caused really big change in runtime behavior.
May be it's just to support ".." in masterfs for getFileObject("..") and findResource?

(In reply to comment #1)
> or implement support for ".." in all filesystems

Assuming it is not overridden, FileObject.getFileObject could implement support for ../ sequences (using getParent), but issue a warning incl. the calling method that this is not supposed to be supported.

Created attachment 105210 [details]
Allow ".." 

Petr Hejl convinced me during today's lunch that it is valuable to get a relative path (possibly including "..") from project.properties and be able to apply it to some file object.

[JG01] masterfs should probably depend on openide.filesystems 7.45 just to be safe.

OK, let's integrate on Monday.

Re. JG01: I don't see much need for such dependency. Classes link properly without it. Functionality of masterfs does not change (compared to 6.9).

ergonomics#2c4b37774c45

Integrated into 'main-golden', will be available in build *201102020000* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/2c4b37774c45
User: Jaroslav Tulach <jtulach@netbeans.org>
Log: #194418: Allow use of .. in getFileObject

*** Bug 195107 has been marked as a duplicate of this bug. ***

implementation added for handling ".." in paths using getCanonical breaks clients working with symlinks. Normalization should be enough.

Created attachment 108133 [details]
normalization instead of getCanonical

Created attachment 108134 [details]
normalization instead of getCanonical - 2

fix in both places

Possibly OK, will need to write a test before integration.

It blocks P2 issue, could you, please, integrate it.

    public void testLinks() throws Exception {
        File tmpDir = new File(System.getProperty("java.io.tmpdir", "/tmp"));
        File testDir = new File(tmpDir, "testLinksDir"); testDir.mkdirs();        
        File dir = new File(testDir, "origDir"); dir.mkdir();
        File file = new File(dir, "origFile"); file.createNewFile();
        File dirLink = new File(dir.getAbsolutePath() + "_link");
        Process exec = Runtime.getRuntime().exec(new String[] {"ln", "-s", dir.getAbsolutePath(), dirLink.getAbsolutePath()});
        exec.waitFor(); exec.destroy();
        FileObject linkDirFO = FileUtil.toFileObject(dirLink);
        String selfName = "../" + dirLink.getName() + "/" + file.getName();
        FileObject fileObject = linkDirFO.getFileObject(selfName);
        FileObject findResource = fileObject.getFileSystem().findResource(dirLink.getAbsolutePath() + "/" + selfName);
        assertEquals(fileObject, findResource);
        assertEquals(linkDirFO, fileObject.getParent());
        exec = Runtime.getRuntime().exec(new String[]{"rm", "-rf", testDir.getAbsolutePath()});
        exec.waitFor(); exec.destroy();
    }

Thanks for the test: ergonomics#cea21310ed68. If the code works, I can backport to 7.0.1.

Integrated into 'main-golden', will be available in build *201105170400* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/ab6416a7509f
User: Jaroslav Tulach <jtulach@netbeans.org>
Log: #194418: Preserve symlinks by using FileUtil.normalizePath

verified.
Please, integrate into 7.0.1

Integrated into 'main-golden', will be available in build *201105190400* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/da8a0c011740
User: Jaroslav Tulach <jtulach@netbeans.org>
Log: More of #194418: Is it possible that invoking ln does not yield non-zero exit code on Windows?

Jarda, why not 
if (isWindows) return;
in the beginning of test?
Windows doesn't support links with "ls -s" anyway

Integrated into 'main-golden', will be available in build *201105200400* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/c07d3dd5f6a8
User: Jaroslav Tulach <jtulach@netbeans.org>
Log: #194418: No links test on windows so far

$ hg exp tip
# Branch release701
# Node ID 473979db499bb6c4b1c2e887ac764550dac6cb6a
# Parent  9bc8448bdf2c576e48c05bb9954a7d346b732997
# Parent  c07d3dd5f6a87ca6fec1bb2f7b3196f4689a34b5
Merge of #194418 to release701

thanks!