Bug 194418 - getFileObject("..") sometimes work, sometimes not
getFileObject("..") sometimes work, sometimes not
Status: VERIFIED FIXED
Product: platform
Classification: Unclassified
Component: Filesystems
7.0
PC Solaris
: P2 (vote)
: 7.0.1
Assigned To: Jaroslav Tulach
issues@platform
: API_REVIEW_FAST
: 195107 (view as bug list)
Depends on:
Blocks: 192807 198347
  Show dependency treegraph
 
Reported: 2011-01-17 16:25 UTC by Vladimir Kvashin
Modified: 2011-05-20 14:08 UTC (History)
8 users (show)

See Also:
Issue Type: DEFECT
:


Attachments
Test to enforce consistency of ".." (2.04 KB, patch)
2011-01-18 12:32 UTC, Jaroslav Tulach
Details | Diff
Allow ".." (12.10 KB, patch)
2011-01-20 19:38 UTC, Jaroslav Tulach
Details | Diff
normalization instead of getCanonical (1.07 KB, patch)
2011-05-05 13:37 UTC, Vladimir Voskresensky
Details | Diff
normalization instead of getCanonical - 2 (2.25 KB, patch)
2011-05-05 13:41 UTC, Vladimir Voskresensky
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vladimir Kvashin 2011-01-17 16:25:20 UTC
For not it's easy to get a file object with not normalized path.
MasterFS allows to get it easily.
After that FileUtil.isParentOf works wrong, see test case below:

public class FileUtilIsParentTestCase extends NbTestCase {

    public FileUtilIsParentTestCase(String testName) {
        super(testName);
    }

    public void testNorm() throws Exception {
        // create common root
        File root = File.createTempFile("normTest", ".dat");
        root.delete();
        root.mkdirs();
        assertTrue(root.exists());
        // create dir1 
        File dir1 = new File(root, "d1");
        dir1.mkdirs();
        FileObject dirFO1 = FileUtil.toFileObject(dir1);
        assertNotNull(dirFO1);
        // create dir2
        File dir2 = new File(root, "d2");
        dir2.mkdirs();
        FileObject dirFO2 = FileUtil.toFileObject(dir2);
        assertNotNull(dirFO2);
               
        // create child in dir2
        File file = new File(dir2, "child");
        file.createNewFile();
        FileObject fo = FileUtil.toFileObject(file);

        // find dir2 by its relative path "../dir2"
        FileObject foundDirFO2 = dirFO1.getFileObject("../" + dirFO2.getNameExt());        
        assertNotNull(foundDirFO2);
        System.out.printf("FOUND: %s\n", foundDirFO2.getPath());
        
        assertParent(dirFO2, fo);
        assertParent(foundDirFO2, fo);
    }
    
    private void assertParent(FileObject parent, FileObject fo) {
        System.err.printf("isParentOf %s %s ? %b\n", parent.getPath(), fo.getPath(), FileUtil.isParentOf(parent, fo));
        assertTrue("FileObject " + parent + " should be parent of " + fo, FileUtil.isParentOf(parent, fo));        
    }
}
Comment 1 Jaroslav Tulach 2011-01-18 12:32:00 UTC
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.
Comment 2 Vladimir Voskresensky 2011-01-18 12:34:39 UTC
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?
Comment 3 Jaroslav Tulach 2011-01-18 16:14:14 UTC
ergonomics#4a6cfcd35eb7
Comment 4 Vladimir Voskresensky 2011-01-18 18:11:00 UTC
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)
Comment 5 Jaroslav Tulach 2011-01-18 18:31:24 UTC
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.
Comment 6 Quality Engineering 2011-01-19 06:35:07 UTC
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
Comment 7 Vladimir Voskresensky 2011-01-19 09:00:53 UTC
(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
Comment 8 Vladimir Voskresensky 2011-01-19 09:03:46 UTC
i.e. you can add 
        // return null for UNC root and paths with ".."
        if(file.getPath().equals("\\\\") || file.getPath().contains("..") {
            return null;
        }
Comment 9 Jaroslav Tulach 2011-01-19 09:57:08 UTC
I let Jesse to address the concern about use of assert in FileUtil.toFileObject. It is his API creation, as far as I know.
Comment 10 Vladimir Kvashin 2011-01-19 11:38:51 UTC
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
Comment 11 Jesse Glick 2011-01-19 15:47:41 UTC
(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.
Comment 12 Petr Hejl 2011-01-19 22:56:05 UTC
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.
Comment 13 Jesse Glick 2011-01-19 23:20:39 UTC
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.
Comment 14 Sergey Petrov 2011-01-19 23:47:23 UTC
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?
Comment 15 Vladimir Voskresensky 2011-01-20 11:50:53 UTC
> 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?
Comment 16 Jesse Glick 2011-01-20 17:31:00 UTC
(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.
Comment 17 Jaroslav Tulach 2011-01-20 19:38:31 UTC
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.
Comment 18 Jesse Glick 2011-01-20 19:47:05 UTC
[JG01] masterfs should probably depend on openide.filesystems 7.45 just to be safe.
Comment 19 Jaroslav Tulach 2011-01-30 18:57:53 UTC
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).
Comment 20 Jaroslav Tulach 2011-01-31 08:48:51 UTC
ergonomics#2c4b37774c45
Comment 21 Quality Engineering 2011-02-02 06:19:09 UTC
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
Comment 22 Marek Fukala 2011-02-08 22:20:58 UTC
*** Bug 195107 has been marked as a duplicate of this bug. ***
Comment 23 Vladimir Voskresensky 2011-05-05 13:36:41 UTC
implementation added for handling ".." in paths using getCanonical breaks clients working with symlinks. Normalization should be enough.
Comment 24 Vladimir Voskresensky 2011-05-05 13:37:25 UTC
Created attachment 108133 [details]
normalization instead of getCanonical
Comment 25 Vladimir Voskresensky 2011-05-05 13:41:15 UTC
Created attachment 108134 [details]
normalization instead of getCanonical - 2

fix in both places
Comment 26 Jaroslav Tulach 2011-05-09 11:00:51 UTC
Possibly OK, will need to write a test before integration.
Comment 27 Vladimir Voskresensky 2011-05-12 10:21:38 UTC
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();
    }
Comment 28 Jaroslav Tulach 2011-05-16 17:10:23 UTC
Thanks for the test: ergonomics#cea21310ed68. If the code works, I can backport to 7.0.1.
Comment 29 Quality Engineering 2011-05-17 10:08:48 UTC
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
Comment 30 Vladimir Voskresensky 2011-05-17 14:23:08 UTC
verified.
Please, integrate into 7.0.1
Comment 31 Quality Engineering 2011-05-19 08:42:13 UTC
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?
Comment 32 Vladimir Voskresensky 2011-05-19 13:30:51 UTC
Jarda, why not 
if (isWindows) return;
in the beginning of test?
Windows doesn't support links with "ls -s" anyway
Comment 33 Quality Engineering 2011-05-20 08:57:11 UTC
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
Comment 34 Jaroslav Tulach 2011-05-20 12:37:05 UTC
$ hg exp tip
# Branch release701
# Node ID 473979db499bb6c4b1c2e887ac764550dac6cb6a
# Parent  9bc8448bdf2c576e48c05bb9954a7d346b732997
# Parent  c07d3dd5f6a87ca6fec1bb2f7b3196f4689a34b5
Merge of #194418 to release701
Comment 35 Vladimir Voskresensky 2011-05-20 14:08:50 UTC
thanks!


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo