From a35a2ca335b5873eb3d465ca6e79212a3750e1e0 Mon Sep 17 00:00:00 2001 From: Suresh Srinivas Date: Thu, 6 Jun 2013 03:29:43 +0000 Subject: [PATCH] HADOOP-8957. Merge r1420965 from trunk git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/branches/branch-2@1490115 13f79535-47bb-0310-9956-ffa450edef68 --- .../hadoop-common/CHANGES.txt | 3 +++ .../apache/hadoop/fs/AbstractFileSystem.java | 14 +++++++--- .../java/org/apache/hadoop/fs/FilterFs.java | 5 ++++ .../apache/hadoop/fs/local/RawLocalFs.java | 8 ++++++ .../apache/hadoop/fs/viewfs/ChRootedFs.java | 9 +++++-- .../hadoop/fs/viewfs/ViewFileSystem.java | 26 +++---------------- .../org/apache/hadoop/fs/viewfs/ViewFs.java | 6 +++++ .../hadoop/fs/viewfs/TestChRootedFs.java | 19 ++++++++++++++ hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt | 3 +++ .../fs/TestHDFSFileContextMainOperations.java | 17 +++++++++++- .../org/apache/hadoop/hdfs/TestDFSUtil.java | 3 +++ 11 files changed, 84 insertions(+), 29 deletions(-) diff --git a/hadoop-common-project/hadoop-common/CHANGES.txt b/hadoop-common-project/hadoop-common/CHANGES.txt index fede590492b..49f8123d6f5 100644 --- a/hadoop-common-project/hadoop-common/CHANGES.txt +++ b/hadoop-common-project/hadoop-common/CHANGES.txt @@ -239,6 +239,9 @@ Release 2.1.0-beta - UNRELEASED HADOOP-9593. stack trace printed at ERROR for all yarn clients without hadoop.home set (stevel) + HADOOP-8957 AbstractFileSystem#IsValidName should be overridden for + embedded file systems like ViewFs (Chris Nauroth via Sanjay Radia) + BREAKDOWN OF HADOOP-8562 SUBTASKS AND RELATED JIRAS HADOOP-8924. Hadoop Common creating package-info.java must not depend on 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 6adbeab60a0..92e3e6a4636 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 abstract class AbstractFileSystem { } /** - * 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 9637b6b913a..cdc0d1fdefa 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 abstract class FilterFs extends AbstractFileSystem { 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 284f432630c..769888c0369 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 @@ -158,6 +158,14 @@ public class RawLocalFs extends DelegateToFileSystem { } } + @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 c99ce3be13b..2c184f6bb05 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 @@ class ChRootedFs extends AbstractFileSystem { 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 @@ class ChRootedFs extends AbstractFileSystem { // 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 221ffd8c5ab..aec87a34c04 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 @@ -64,6 +64,9 @@ import org.apache.hadoop.util.Time; @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( @@ -98,23 +101,6 @@ public class ViewFileSystem extends FileSystem { 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 @@ -126,10 +112,6 @@ public class ViewFileSystem extends FileSystem { 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; } @@ -689,7 +671,7 @@ public class ViewFileSystem extends FileSystem { 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 dcfe5f32031..2bbdc164f87 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 class ViewFs extends AbstractFileSystem { 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 6078bc0caa2..e639291c65b 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 java.util.EnumSet; 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.After; import org.junit.Assert; import org.junit.Before; import org.junit.Test; +import org.mockito.Mockito; public class TestChRootedFs { FileContextTestHelper fileContextTestHelper = new FileContextTestHelper(); @@ -308,4 +310,21 @@ public class TestChRootedFs { 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 a71b31c897a..4b313ccaf0f 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt +++ b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt @@ -345,6 +345,9 @@ Release 2.1.0-beta - UNRELEASED HDFS-4815. TestRBWBlockInvalidation: Double call countReplicas() to fetch corruptReplicas and liveReplicas is not needed. (Tian Hong Wang via atm) + HADOOP-8957 HDFS tests for AbstractFileSystem#IsValidName should be overridden for + embedded file systems like ViewFs (Chris Nauroth via Sanjay Radia) + BREAKDOWN OF HDFS-347 SUBTASKS AND RELATED JIRAS HDFS-4353. Encapsulate connections to peers in Peer and PeerServer classes. 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 2b58a079254..d1193f85ef8 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 @@ -257,7 +257,22 @@ public class TestHDFSFileContextMainOperations extends 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 a4fadc5e82f..5143889fb73 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 @@ -640,9 +640,12 @@ public class TestDFSUtil { @Test (timeout=15000) 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")); } @Test(timeout=5000)