diff --git a/hadoop-common-project/hadoop-common/CHANGES.txt b/hadoop-common-project/hadoop-common/CHANGES.txt index 84222ad0d4..eb44714da2 100644 --- a/hadoop-common-project/hadoop-common/CHANGES.txt +++ b/hadoop-common-project/hadoop-common/CHANGES.txt @@ -298,6 +298,9 @@ Trunk (Unreleased) HADOOP-9131. Turn off TestLocalFileSystem#testListStatusWithColons on Windows. (Chris Nauroth via suresh) + HADOOP-8957 AbstractFileSystem#IsValidName should be overridden for + embedded file systems like ViewFs (Chris Nauroth via Sanjay Radia) + OPTIMIZATIONS HADOOP-7761. Improve the performance of raw comparisons. (todd) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/AbstractFileSystem.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/AbstractFileSystem.java index 6adbeab60a..92e3e6a463 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/AbstractFileSystem.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/AbstractFileSystem.java @@ -85,14 +85,20 @@ public Statistics getStatistics() { } /** - * Prohibits names which contain a ".", "..", ":" or "/" + * Returns true if the specified string is considered valid in the path part + * of a URI by this file system. The default implementation enforces the rules + * of HDFS, but subclasses may override this method to implement specific + * validation rules for specific file systems. + * + * @param src String source filename to check, path part of the URI + * @return boolean true if the specified string is considered valid */ - private static boolean isValidName(String src) { - // Check for ".." "." ":" "/" + public boolean isValidName(String src) { + // Prohibit ".." "." and anything containing ":" StringTokenizer tokens = new StringTokenizer(src, Path.SEPARATOR); while(tokens.hasMoreTokens()) { String element = tokens.nextToken(); - if (element.equals("target/generated-sources") || + if (element.equals("..") || element.equals(".") || (element.indexOf(":") >= 0)) { return false; diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FilterFs.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FilterFs.java index 9637b6b913..cdc0d1fdef 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FilterFs.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FilterFs.java @@ -278,4 +278,9 @@ public String getCanonicalServiceName() { public List> getDelegationTokens(String renewer) throws IOException { return myFs.getDelegationTokens(renewer); } + + @Override + public boolean isValidName(String src) { + return myFs.isValidName(src); + } } diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/local/RawLocalFs.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/local/RawLocalFs.java index b9a9277ade..9ce0a97ab1 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/local/RawLocalFs.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/local/RawLocalFs.java @@ -159,6 +159,14 @@ public FileStatus getFileLinkStatus(final Path f) throws IOException { } } + @Override + public boolean isValidName(String src) { + // Different local file systems have different validation rules. Skip + // validation here and just let the OS handle it. This is consistent with + // RawLocalFileSystem. + return true; + } + @Override public Path getLinkTarget(Path f) throws IOException { /* We should never get here. Valid local links are resolved transparently diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ChRootedFs.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ChRootedFs.java index c99ce3be13..2c184f6bb0 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ChRootedFs.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ChRootedFs.java @@ -83,7 +83,12 @@ protected Path fullPath(final Path path) { return new Path((chRootPathPart.isRoot() ? "" : chRootPathPartString) + path.toUri().getPath()); } - + + @Override + public boolean isValidName(String src) { + return myFs.isValidName(fullPath(new Path(src)).toUri().toString()); + } + public ChRootedFs(final AbstractFileSystem fs, final Path theRoot) throws URISyntaxException { super(fs.getUri(), fs.getUri().getScheme(), @@ -103,7 +108,7 @@ public ChRootedFs(final AbstractFileSystem fs, final Path theRoot) // scheme:/// and scheme://authority/ myUri = new URI(myFs.getUri().toString() + (myFs.getUri().getAuthority() == null ? "" : Path.SEPARATOR) + - chRootPathPart.toString().substring(1)); + chRootPathPart.toUri().getPath().substring(1)); super.checkPath(theRoot); } diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java index f4fbc66b53..3f8864203d 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java @@ -62,6 +62,9 @@ @InterfaceAudience.Public @InterfaceStability.Evolving /*Evolving for a release,to be changed to Stable */ public class ViewFileSystem extends FileSystem { + + private static final Path ROOT_PATH = new Path(Path.SEPARATOR); + static AccessControlException readOnlyMountTable(final String operation, final String p) { return new AccessControlException( @@ -96,23 +99,6 @@ URI[] getTargets() { InodeTree fsState; // the fs state; ie the mount table Path homeDir = null; - /** - * Prohibits names which contain a ".", "..", ":" or "/" - */ - private static boolean isValidName(final String src) { - // Check for ".." "." ":" "/" - final StringTokenizer tokens = new StringTokenizer(src, Path.SEPARATOR); - while(tokens.hasMoreTokens()) { - String element = tokens.nextToken(); - if (element.equals("..") || - element.equals(".") || - (element.indexOf(":") >= 0)) { - return false; - } - } - return true; - } - /** * Make the path Absolute and get the path-part of a pathname. * Checks that URI matches this file system @@ -124,10 +110,6 @@ private static boolean isValidName(final String src) { private String getUriPath(final Path p) { checkPath(p); String s = makeAbsolute(p).toUri().getPath(); - if (!isValidName(s)) { - throw new InvalidPathException("Path part " + s + " from URI" + p - + " is not a valid filename."); - } return s; } @@ -672,7 +654,7 @@ public FileStatus getFileStatus(Path f) throws IOException { PERMISSION_RRR, ugi.getUserName(), ugi.getGroupNames()[0], new Path(theInternalDir.fullPath).makeQualified( - myUri, null)); + myUri, ROOT_PATH)); } diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFs.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFs.java index dcfe5f3203..2bbdc164f8 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFs.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFs.java @@ -597,6 +597,12 @@ public List> getDelegationTokens(String renewer) throws IOException { return result; } + @Override + public boolean isValidName(String src) { + // Prefix validated at mount time and rest of path validated by mount target. + return true; + } + /* diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/TestChRootedFs.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/TestChRootedFs.java index c52280154d..f458ec1499 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/TestChRootedFs.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/TestChRootedFs.java @@ -25,6 +25,7 @@ import static org.apache.hadoop.fs.FileContextTestHelper.*; import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.AbstractFileSystem; import org.apache.hadoop.fs.CreateFlag; import org.apache.hadoop.fs.FileContext; import org.apache.hadoop.fs.FileContextTestHelper; @@ -36,6 +37,7 @@ import org.junit.Assert; import org.junit.Before; import org.junit.Test; +import org.mockito.Mockito; public class TestChRootedFs { FileContext fc; // The ChRoootedFs @@ -307,4 +309,21 @@ public void testResolvePathNonExisting() throws IOException { fc.getDefaultFileSystem().resolvePath(new Path("/nonExisting")); } + @Test + public void testIsValidNameValidInBaseFs() throws Exception { + AbstractFileSystem baseFs = Mockito.spy(fc.getDefaultFileSystem()); + ChRootedFs chRootedFs = new ChRootedFs(baseFs, new Path("/chroot")); + Mockito.doReturn(true).when(baseFs).isValidName(Mockito.anyString()); + Assert.assertTrue(chRootedFs.isValidName("/test")); + Mockito.verify(baseFs).isValidName("/chroot/test"); + } + + @Test + public void testIsValidNameInvalidInBaseFs() throws Exception { + AbstractFileSystem baseFs = Mockito.spy(fc.getDefaultFileSystem()); + ChRootedFs chRootedFs = new ChRootedFs(baseFs, new Path("/chroot")); + Mockito.doReturn(false).when(baseFs).isValidName(Mockito.anyString()); + Assert.assertFalse(chRootedFs.isValidName("/test")); + Mockito.verify(baseFs).isValidName("/chroot/test"); + } } diff --git a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt index 596e4167e6..a1fc5c157a 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt +++ b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt @@ -274,6 +274,10 @@ Trunk (Unreleased) HDFS-4269. Datanode rejects all datanode registrations from localhost in single-node developer setup on Windows. (Chris Nauroth via suresh) + HADOOP-8957 HDFS tests for AbstractFileSystem#IsValidName should be overridden for + embedded file systems like ViewFs (Chris Nauroth via Sanjay Radia) + + Release 2.0.3-alpha - Unreleased INCOMPATIBLE CHANGES diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/fs/TestHDFSFileContextMainOperations.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/fs/TestHDFSFileContextMainOperations.java index 018d3886a6..c5250023e4 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/fs/TestHDFSFileContextMainOperations.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/fs/TestHDFSFileContextMainOperations.java @@ -255,7 +255,22 @@ public void testEditsLogRename() throws Exception { Assert.assertFalse(fs.exists(src1)); // ensure src1 is already renamed Assert.assertTrue(fs.exists(dst1)); // ensure rename dst exists } - + + @Test + public void testIsValidNameInvalidNames() { + String[] invalidNames = { + "/foo/../bar", + "/foo/./bar", + "/foo/:/bar", + "/foo:bar" + }; + + for (String invalidName: invalidNames) { + Assert.assertFalse(invalidName + " is not valid", + fc.getDefaultFileSystem().isValidName(invalidName)); + } + } + private void oldRename(Path src, Path dst, boolean renameSucceeds, boolean exception) throws Exception { DistributedFileSystem fs = (DistributedFileSystem) cluster.getFileSystem(); diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUtil.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUtil.java index 06b4ed0cb8..d59acfa6ec 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUtil.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUtil.java @@ -624,8 +624,11 @@ public void testGetNNUris() throws Exception { @Test public void testIsValidName() { assertFalse(DFSUtil.isValidName("/foo/../bar")); + assertFalse(DFSUtil.isValidName("/foo/./bar")); assertFalse(DFSUtil.isValidName("/foo//bar")); assertTrue(DFSUtil.isValidName("/")); assertTrue(DFSUtil.isValidName("/bar/")); + assertFalse(DFSUtil.isValidName("/foo/:/bar")); + assertFalse(DFSUtil.isValidName("/foo:bar")); } }