7006576: (zipfs) Path.exists() always returns false on dirs when zip/JAR file built without dirs
authorsherman
Mon, 24 Jan 2011 11:47:06 -0800
changeset 8005 5bc99c45810a
parent 8004 11f3cdd5c639
child 8006 79a66d3d5e77
child 8151 88b01a6d5f51
child 8154 cbb914db1110
7006576: (zipfs) Path.exists() always returns false on dirs when zip/JAR file built without dirs 7009092: (zipfs) ZipPath.isSameFile() should always return true if this Path and the given Path are equal. 7009085: (zipfs) ZipPath.normalize("/./.") returns null. 7009102: (zipfs) ZipPath.toRealPath() should always return absolute path. Summary: zip filesystem provider update Reviewed-by: alanb
jdk/src/share/demo/nio/zipfs/src/com/sun/nio/zipfs/ZipFileSystem.java
jdk/src/share/demo/nio/zipfs/src/com/sun/nio/zipfs/ZipPath.java
jdk/test/demo/zipfs/PathOps.java
jdk/test/demo/zipfs/ZipFSTester.java
jdk/test/demo/zipfs/basic.sh
--- a/jdk/src/share/demo/nio/zipfs/src/com/sun/nio/zipfs/ZipFileSystem.java	Sat Jan 22 08:43:25 2011 -0500
+++ b/jdk/src/share/demo/nio/zipfs/src/com/sun/nio/zipfs/ZipFileSystem.java	Mon Jan 24 11:47:06 2011 -0800
@@ -78,7 +78,6 @@
     // configurable by env map
     private final String  defaultDir;    // default dir for the file system
     private final String  nameEncoding;  // default encoding for name/comment
-    private final boolean buildDirTree;  // build a dir tree for directoryStream ops
     private final boolean useTempFile;   // use a temp file for newOS, default
                                          // is to use BAOS for better performance
     private final boolean createNew;     // create a new zip if not exists
@@ -94,7 +93,6 @@
         this.createNew    = "true".equals(env.get("create"));
         this.nameEncoding = env.containsKey("encoding") ?
                             (String)env.get("encoding") : "UTF-8";
-        this.buildDirTree = TRUE.equals(env.get("buildDirTreea"));
         this.useTempFile  = TRUE.equals(env.get("useTempFile"));
         this.defaultDir   = env.containsKey("default.dir") ?
                             (String)env.get("default.dir") : "/";
@@ -290,28 +288,16 @@
         try {
             ensureOpen();
             e = getEntry0(path);
-        } finally {
-            endRead();
-        }
-        if (e == null) {
-            if (path.length == 0) {
-                e = new Entry(new byte[0]);  // root
-            } else if (buildDirTree) {
-                IndexNode inode = getDirs().get(IndexNode.keyOf(path));
+            if (e == null) {
+                IndexNode inode = getInode(path);
                 if (inode == null)
                     return null;
-                e = new Entry(inode.name);
-            } else {
-                return null;
+                e = new Entry(inode.name);       // pseudo directory
+                e.method = METHOD_STORED;        // STORED for dir
+                e.mtime = e.atime = e.ctime = -1;// -1 for all times
             }
-            e.method = METHOD_STORED;        // STORED for dir
-            BasicFileAttributes bfas = Attributes.readBasicFileAttributes(zfpath);
-            if (bfas.lastModifiedTime() != null)
-                e.mtime = bfas.lastModifiedTime().toMillis();
-            if (bfas.lastAccessTime() != null)
-                e.atime = bfas.lastAccessTime().toMillis();
-            if (bfas.creationTime() != null)
-                e.ctime = bfas.creationTime().toMillis();
+        } finally {
+            endRead();
         }
         return new ZipFileAttributes(e);
     }
@@ -346,7 +332,7 @@
         beginRead();
         try {
             ensureOpen();
-            return getEntry0(path) != null;
+            return getInode(path) != null;
         } finally {
             endRead();
         }
@@ -355,13 +341,10 @@
     boolean isDirectory(byte[] path)
         throws IOException
     {
-        if (buildDirTree)
-            return getDirs().containsKey(IndexNode.keyOf(path));
-
         beginRead();
         try {
-            Entry e = getEntry0(path);
-            return (e != null && e.isDir()) || path.length == 0;
+            IndexNode n = getInode(path);
+            return n != null && n.isDir();
         } finally {
             endRead();
         }
@@ -383,39 +366,16 @@
         beginWrite();    // iteration of inodes needs exclusive lock
         try {
             ensureOpen();
-            if (buildDirTree) {
-                IndexNode inode = getDirs().get(IndexNode.keyOf(path));
-                if (inode == null)
-                    throw new NotDirectoryException(getString(path));
-                List<Path> list = new ArrayList<>();
-                IndexNode child = inode.child;
-                while (child != null) {
-                    ZipPath zp = toZipPath(child.name);
-                    if (filter == null || filter.accept(zp))
-                        list.add(zp);
-                    child = child.sibling;
-                }
-                return list.iterator();
-            }
-
-            if (!isDirectory(path))
+            IndexNode inode = getInode(path);
+            if (inode == null)
                 throw new NotDirectoryException(getString(path));
             List<Path> list = new ArrayList<>();
-            path = toDirectoryPath(path);
-            for (IndexNode key : inodes.keySet()) {
-                if (!isParentOf(path, key.name))  // is "path" the parent of "name"
-                    continue;
-                int off = path.length;
-                while (off < key.name.length) {
-                    if (key.name[off] == '/')
-                        break;
-                    off++;
-                }
-                if (off < (key.name.length - 1))
-                    continue;
-                ZipPath zp = toZipPath(key.name);
+            IndexNode child = inode.child;
+            while (child != null) {
+                ZipPath zp = toZipPath(child.name);
                 if (filter == null || filter.accept(zp))
                     list.add(zp);
+                child = child.sibling;
             }
             return list.iterator();
         } finally {
@@ -433,7 +393,6 @@
             ensureOpen();
             if (dir.length == 0 || exists(dir))  // root dir, or exiting dir
                 throw new FileAlreadyExistsException(getString(dir));
-
             checkParents(dir);
             Entry e = new Entry(dir, Entry.NEW);
             e.method = METHOD_STORED;            // STORED for dir
@@ -476,7 +435,7 @@
                 checkParents(dst);
             }
             Entry u = new Entry(eSrc, Entry.COPY);    // copy eSrc entry
-            u.name = dst;                             // change name
+            u.name(dst);                              // change name
             if (eSrc.type == Entry.NEW || eSrc.type == Entry.FILECH)
             {
                 u.type = eSrc.type;    // make it the same type
@@ -518,7 +477,7 @@
             if (opt == APPEND)
                 hasAppend = true;
         }
-        beginRead();    // only need a readlock, the "update()" will
+        beginRead();                 // only need a readlock, the "update()" will
         try {                        // try to obtain a writelock when the os is
             ensureOpen();            // being closed.
             Entry e = getEntry0(path);
@@ -869,43 +828,27 @@
     private void checkParents(byte[] path) throws IOException {
         beginRead();
         try {
-            while ((path = getParent(path)) != null) {
-                if (!inodes.containsKey(IndexNode.keyOf(path)))
+            while ((path = getParent(path)) != null && path.length != 0) {
+                if (!inodes.containsKey(IndexNode.keyOf(path))) {
                     throw new NoSuchFileException(getString(path));
+                }
             }
         } finally {
             endRead();
         }
     }
 
+    private static byte[] ROOTPATH = new byte[0];
     private static byte[] getParent(byte[] path) {
         int off = path.length - 1;
         if (off > 0 && path[off] == '/')  // isDirectory
             off--;
         while (off > 0 && path[off] != '/') { off--; }
-        if (off == 0)
-            return null;                  // top entry
+        if (off <= 0)
+            return ROOTPATH;
         return Arrays.copyOf(path, off + 1);
     }
 
-    // If "starter" is the parent directory of "path"
-    private static boolean isParentOf(byte[] p, byte[] c) {
-        final int plen = p.length;
-        if (plen == 0)          // root dir
-            return true;
-        if (plen  >= c.length)
-            return false;
-        int n = 0;
-        while (n < plen) {
-            if (p[n] != c[n])
-                return false;
-            n++;
-        }
-        if (p[n - 1] != '/' && (c[n] != '/' || n == c.length - 1))
-            return false;
-        return true;
-    }
-
     private final void beginWrite() {
         rwlock.writeLock().lock();
     }
@@ -926,7 +869,7 @@
 
     private volatile boolean isOpen = true;
     private final SeekableByteChannel ch; // channel to the zipfile
-    final byte[]  cen;           // CEN & ENDHDR
+    final byte[]  cen;     // CEN & ENDHDR
     private END  end;
     private long locpos;   // position of first LOC header (usually 0)
 
@@ -1058,6 +1001,7 @@
         if (end.endpos == 0) {
             inodes = new LinkedHashMap<>(10);
             locpos = 0;
+            buildNodeTree();
             return null;         // only END header present
         }
         if (end.cenlen > end.endpos)
@@ -1101,6 +1045,7 @@
         if (pos + ENDHDR != cen.length) {
             zerror("invalid CEN header (bad header size)");
         }
+        buildNodeTree();
         return cen;
     }
 
@@ -1125,12 +1070,15 @@
 
     private boolean hasUpdate = false;
 
+    // shared key. consumer guarantees the "writeLock" before use it.
+    private final IndexNode LOOKUPKEY = IndexNode.keyOf(null);
+
     private void updateDelete(Entry e) {
         beginWrite();
         try {
-            inodes.remove(IndexNode.keyOf(e.name));  //inodes.remove(e.name);
+            removeFromTree(e);
+            inodes.remove(e);
             hasUpdate = true;
-            dirs = null;
         } finally {
              endWrite();
         }
@@ -1139,9 +1087,16 @@
     private void update(Entry e) {
         beginWrite();
         try {
-            inodes.put(IndexNode.keyOf(e.name), e);  //inodes.put(e, e);
+            IndexNode old = inodes.put(e, e);
+            if (old != null) {
+                removeFromTree(old);
+            }
+            if (e.type == Entry.NEW || e.type == Entry.FILECH) {
+                IndexNode parent = inodes.get(LOOKUPKEY.as(getParent(e.name)));
+                e.sibling = parent.child;
+                parent.child = e;
+            }
             hasUpdate = true;
-            dirs = null;
         } finally {
             endWrite();
         }
@@ -1229,7 +1184,7 @@
                         // ext) without enflating/deflating from the old zip
                         // file LOC entry.
                         written += copyLOCEntry(e, true, os, written, buf);
-                    } else {                          // NEW or FILECH
+                    } else {                          // NEW, FILECH or CEN
                         e.locoff = written;
                         written += e.writeLOC(os);    // write loc header
                         if (e.bytes != null) {        // in-memory, deflated
@@ -1265,7 +1220,10 @@
                 } catch (IOException x) {
                     x.printStackTrace();    // skip any in-accurate entry
                 }
-            } else {    // unchanged inode
+            } else {                        // unchanged inode
+                if (inode.pos == -1) {
+                    continue;               // pseudo directory node
+                }
                 e = Entry.readCEN(this, inode.pos);
                 try {
                     written += copyLOCEntry(e, false, os, written, buf);
@@ -1318,34 +1276,28 @@
         //System.out.printf("->sync(%s) done!%n", toString());
     }
 
-    private Entry getEntry0(byte[] path) throws IOException {
+    private IndexNode getInode(byte[] path) {
         if (path == null)
             throw new NullPointerException("path");
-        if (path.length == 0)
-            return null;
-        IndexNode inode = null;
         IndexNode key = IndexNode.keyOf(path);
-        if ((inode = inodes.get(key)) == null) {
-            if (path[path.length -1] == '/')  // already has a slash
-                return null;
+        IndexNode inode = inodes.get(key);
+        if (inode == null &&
+            (path.length == 0 || path[path.length -1] != '/')) {
+            // if does not ends with a slash
             path = Arrays.copyOf(path, path.length + 1);
             path[path.length - 1] = '/';
-            if ((inode = inodes.get(key.as(path))) == null)
-                return null;
+            inode = inodes.get(key.as(path));
         }
-        if (inode instanceof Entry)
-            return (Entry)inode;
-        return Entry.readCEN(this, inode.pos);
+        return inode;
     }
 
-    // Test if the "name" a parent directory of any entry (dir empty)
-    boolean isAncestor(byte[] name) {
-        for (Map.Entry<IndexNode, IndexNode> entry : inodes.entrySet()) {
-            byte[] ename = entry.getKey().name;
-            if (isParentOf(name, ename))
-                return true;
-        }
-        return false;
+    private Entry getEntry0(byte[] path) throws IOException {
+        IndexNode inode = getInode(path);
+        if (inode instanceof Entry)
+            return (Entry)inode;
+        if (inode == null || inode.pos == -1)
+            return null;
+        return Entry.readCEN(this, inode.pos);
     }
 
     public void deleteFile(byte[] path, boolean failIfNotExists)
@@ -1359,7 +1311,7 @@
             if (failIfNotExists)
                 throw new NoSuchFileException(getString(path));
         } else {
-            if (e.isDir() && isAncestor(path))
+            if (e.isDir() && e.child != null)
                 throw new DirectoryNotEmptyException(getString(path));
             updateDelete(e);
         }
@@ -1539,7 +1491,6 @@
         public int available() {
             return rem > Integer.MAX_VALUE ? Integer.MAX_VALUE : (int) rem;
         }
-
         public long size() {
             return size;
         }
@@ -1762,7 +1713,7 @@
         int    pos = -1;  // postion in cen table, -1 menas the
                           // entry does not exists in zip file
         IndexNode(byte[] name, int pos) {
-            as(name);
+            name(name);
             this.pos = pos;
         }
 
@@ -1770,15 +1721,25 @@
             return new IndexNode(name, -1);
         }
 
-        final IndexNode as(byte[] name) {    // reuse the node, mostly
-            this.name = name;                // as a lookup "key"
+        final void name(byte[] name) {
+            this.name = name;
             this.hashcode = Arrays.hashCode(name);
+        }
+
+        final IndexNode as(byte[] name) {           // reuse the node, mostly
+            name(name);                             // as a lookup "key"
             return this;
         }
 
+        boolean isDir() {
+            return name != null &&
+                   (name.length == 0 || name[name.length - 1] == '/');
+        }
+
         public boolean equals(Object other) {
-            if (!(other instanceof IndexNode))
+            if (!(other instanceof IndexNode)) {
                 return false;
+            }
             return Arrays.equals(name, ((IndexNode)other).name);
         }
 
@@ -1798,6 +1759,7 @@
         static final int FILECH = 3;    // fch update in "file"
         static final int COPY   = 4;    // copy of a CEN entry
 
+
         byte[] bytes;      // updated content bytes
         Path   file;       // use tmp file to store bytes;
         int    type = CEN; // default is the entry read from cen
@@ -1825,7 +1787,7 @@
         Entry() {}
 
         Entry(byte[] name) {
-            this.name   = name;
+            name(name);
             this.mtime  = System.currentTimeMillis();
             this.crc    = 0;
             this.size   = 0;
@@ -1839,8 +1801,8 @@
         }
 
         Entry (Entry e, int type) {
+            name(e.name);
             this.version   = e.version;
-            this.name      = e.name;
             this.ctime     = e.ctime;
             this.atime     = e.atime;
             this.mtime     = e.mtime;
@@ -1855,7 +1817,6 @@
             this.attrsEx   = e.attrsEx;
             this.locoff    = e.locoff;
             this.comment   = e.comment;
-
             this.type      = type;
         }
 
@@ -1865,12 +1826,6 @@
             this.method = METHOD_STORED;
         }
 
-        boolean isDir() {
-            return name != null &&
-                   (name.length == 0 ||
-                    name[name.length - 1] == '/');
-        }
-
         int version() throws ZipException {
             if (method == METHOD_DEFLATED)
                 return 20;
@@ -1909,7 +1864,7 @@
             locoff      = CENOFF(cen, pos);
 
             pos += CENHDR;
-            name = Arrays.copyOfRange(cen, pos, pos + nlen);
+            name(Arrays.copyOfRange(cen, pos, pos + nlen));
 
             pos += nlen;
             if (elen > 0) {
@@ -2132,7 +2087,8 @@
                 }
                 writeShort(os, flag);           // general purpose bit flag
                 writeShort(os, method);         // compression method
-                writeInt(os, mtime);            // last modification time
+                                                // last modification time
+                writeInt(os, (int)javaToDosTime(mtime));
                 writeInt(os, crc);              // crc-32
                 if (elen64 != 0) {
                     writeInt(os, ZIP64_MINVAL);
@@ -2318,50 +2274,56 @@
     //     structure.
     // A possible solution is to build the node tree ourself as
     // implemented below.
-    private HashMap<IndexNode, IndexNode> dirs;
     private IndexNode root;
-    private IndexNode addToDir(IndexNode child) {
-        IndexNode cinode = dirs.get(child);
-        if (cinode != null)
-            return cinode;
-
-        byte[] cname = child.name;
-        byte[] pname = getParent(cname);
-        IndexNode pinode;
 
-        if (pname != null)
-            pinode = addToDir(IndexNode.keyOf(pname));
-        else
-            pinode = root;
-        cinode = inodes.get(child);
-        if (cname[cname.length -1] != '/') {  // not a dir
-            cinode.sibling = pinode.child;
-            pinode.child = cinode;
-            return null;
+    private void addToTree(IndexNode inode, HashSet<IndexNode> dirs) {
+        if (dirs.contains(inode)) {
+            return;
         }
-        //cinode = dirs.get(child);
-        if (cinode == null)       // pseudo directry entry
-            cinode = new IndexNode(cname, -1);
-        cinode.sibling = pinode.child;
-        pinode.child = cinode;
-
-        dirs.put(child, cinode);
-        return cinode;
+        IndexNode parent;
+        byte[] name = inode.name;
+        byte[] pname = getParent(name);
+        if (inodes.containsKey(LOOKUPKEY.as(pname))) {
+            parent = inodes.get(LOOKUPKEY);
+        } else {    // pseudo directory entry
+            parent = new IndexNode(pname, -1);
+            inodes.put(parent, parent);
+        }
+        addToTree(parent, dirs);
+        inode.sibling = parent.child;
+        parent.child = inode;
+        if (name[name.length -1] == '/')
+            dirs.add(inode);
     }
 
-    private HashMap<IndexNode, IndexNode> getDirs()
-        throws IOException
-    {
+    private void removeFromTree(IndexNode inode) {
+        IndexNode parent = inodes.get(LOOKUPKEY.as(getParent(inode.name)));
+        IndexNode child = parent.child;
+        if (child == inode) {
+            parent.child = child.sibling;
+        } else {
+            IndexNode last = child;
+            while ((child = child.sibling) != null) {
+                if (child == inode) {
+                    last.sibling = child.sibling;
+                    break;
+                } else {
+                    last = child;
+                }
+            }
+        }
+    }
+
+    private void buildNodeTree() throws IOException {
         beginWrite();
         try {
-            if (dirs != null)
-                return dirs;
-            dirs = new HashMap<>();
-            root = new IndexNode(new byte[0], -1);
-            dirs.put(root, root);
-            for (IndexNode node : inodes.keySet())
-                addToDir(node);
-            return dirs;
+            HashSet<IndexNode> dirs = new HashSet<>();
+            IndexNode root = new IndexNode(ROOTPATH, -1);
+            inodes.put(root, root);
+            dirs.add(root);
+            for (IndexNode node : inodes.keySet().toArray(new IndexNode[0])) {
+                addToTree(node, dirs);
+            }
         } finally {
             endWrite();
         }
--- a/jdk/src/share/demo/nio/zipfs/src/com/sun/nio/zipfs/ZipPath.java	Sat Jan 22 08:43:25 2011 -0500
+++ b/jdk/src/share/demo/nio/zipfs/src/com/sun/nio/zipfs/ZipPath.java	Mon Jan 24 11:47:06 2011 -0800
@@ -157,7 +157,7 @@
 
     @Override
     public ZipPath toRealPath(boolean resolveLinks) throws IOException {
-        ZipPath realPath = new ZipPath(zfs, getResolvedPath());
+        ZipPath realPath = new ZipPath(zfs, getResolvedPath()).toAbsolutePath();
         realPath.checkAccess();
         return realPath;
     }
@@ -472,8 +472,11 @@
             int n = offsets[i];
             int len = (i == offsets.length - 1)?
                       (path.length - n):(offsets[i + 1] - n - 1);
-            if (len == 1 && path[n] == (byte)'.')
+            if (len == 1 && path[n] == (byte)'.') {
+                if (m == 0 && path[0] == '/')   // absolute path
+                    to[m++] = '/';
                 continue;
+            }
             if (len == 2 && path[n] == '.' && path[n + 1] == '.') {
                 if (lastMOff >= 0) {
                     m = lastM[lastMOff--];  // retreat
@@ -726,6 +729,8 @@
 
     @Override
     public boolean isSameFile(Path other) throws IOException {
+        if (this.equals(other))
+            return true;
         if (other == null ||
             this.getFileSystem() != other.getFileSystem())
             return false;
--- a/jdk/test/demo/zipfs/PathOps.java	Sat Jan 22 08:43:25 2011 -0500
+++ b/jdk/test/demo/zipfs/PathOps.java	Mon Jan 24 11:47:06 2011 -0800
@@ -193,6 +193,17 @@
         return this;
     }
 
+    PathOps isSameFile(String target) {
+        try {
+            out.println("check two paths are same");
+            checkPath();
+            check(path.isSameFile(test(target).path()), true);
+        } catch (IOException ioe) {
+            fail();
+        }
+        return this;
+    }
+
     PathOps invalid() {
         if (!(exc instanceof InvalidPathException)) {
             out.println("InvalidPathException not thrown as expected");
@@ -344,7 +355,12 @@
             .normalize("foo");
         test("/foo/bar/gus/../..")
             .normalize("/foo");
-
+        test("/./.")
+            .normalize("/");
+        test("/.")
+            .normalize("/");
+        test("/./abc")
+            .normalize("/abc");
         // invalid
         test("foo\u0000bar")
             .invalid();
@@ -365,6 +381,10 @@
             .root("/")
             .parent("/foo")
             .name("bar");
+
+        // isSameFile
+        test("/fileDoesNotExist")
+            .isSameFile("/fileDoesNotExist");
     }
 
     static void npes() {
--- a/jdk/test/demo/zipfs/ZipFSTester.java	Sat Jan 22 08:43:25 2011 -0500
+++ b/jdk/test/demo/zipfs/ZipFSTester.java	Mon Jan 24 11:47:06 2011 -0800
@@ -28,6 +28,7 @@
 import java.nio.file.attribute.*;
 import java.net.*;
 import java.util.*;
+import java.util.zip.*;
 
 import static java.nio.file.StandardOpenOption.*;
 import static java.nio.file.StandardCopyOption.*;
@@ -42,7 +43,8 @@
         FileSystem fs = null;
         try {
             fs = newZipFileSystem(Paths.get(args[0]), new HashMap<String, Object>());
-            test(fs);
+            test0(fs);
+            test1(fs);
             test2(fs);   // more tests
         } finally {
             if (fs != null)
@@ -50,11 +52,31 @@
         }
     }
 
-    static void test(FileSystem fs)
+    static void test0(FileSystem fs)
+        throws Exception
+    {
+        List<String> list = new LinkedList<>();
+        try (ZipFile zf = new ZipFile(fs.toString())) {
+            Enumeration<? extends ZipEntry> zes = zf.entries();
+            while (zes.hasMoreElements()) {
+                list.add(zes.nextElement().getName());
+            }
+            for (String pname : list) {
+                Path path = fs.getPath(pname);
+                if (!path.exists())
+                    throw new RuntimeException("path existence check failed!");
+                while ((path = path.getParent()) != null) {
+                    if (!path.exists())
+                        throw new RuntimeException("parent existence check failed!");
+                }
+            }
+        }
+    }
+
+    static void test1(FileSystem fs)
         throws Exception
     {
         Random rdm = new Random();
-
         // clone a fs and test on it
         Path tmpfsPath = getTempPath();
         Map<String, Object> env = new HashMap<String, Object>();
--- a/jdk/test/demo/zipfs/basic.sh	Sat Jan 22 08:43:25 2011 -0500
+++ b/jdk/test/demo/zipfs/basic.sh	Mon Jan 24 11:47:06 2011 -0800
@@ -21,7 +21,7 @@
 # questions.
 #
 # @test
-# @bug 6990846
+# @bug 6990846 7009092 7009085
 # @summary Test ZipFileSystem demo
 # @build Basic PathOps ZipFSTester
 # @run shell basic.sh