From e333072144a8da126fe93ce9c194bbb1d9ddf433 Mon Sep 17 00:00:00 2001 From: Suresh Srinivas Date: Wed, 28 Apr 2010 05:45:40 +0000 Subject: [PATCH] HADOOP-6703. Prevent renaming a file, directory or symbolic link to itself. Contributed by Eli Collins. git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/trunk@938788 13f79535-47bb-0310-9956-ffa450edef68 --- CHANGES.txt | 3 + .../apache/hadoop/fs/AbstractFileSystem.java | 8 ++ .../fs/FileContextMainOperationsBaseTest.java | 40 +++++++ .../hadoop/fs/FileContextSymlinkBaseTest.java | 104 +++++++++++++++++- 4 files changed, 152 insertions(+), 3 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 37a13e9919e..2a431a9bbea 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -361,6 +361,9 @@ Trunk (unreleased changes) HADOOP-6690. FilterFileSystem correctly handles setTimes call. (Rodrigo Schmidt via dhruba) + HADOOP-6703. Prevent renaming a file, directory or symbolic link to + itself. (Eli Collins via suresh) + HADOOP-6710. Symbolic umask for file creation is not conformant with posix. (suresh) diff --git a/src/java/org/apache/hadoop/fs/AbstractFileSystem.java b/src/java/org/apache/hadoop/fs/AbstractFileSystem.java index f1d6530e93f..e68fb47cda3 100644 --- a/src/java/org/apache/hadoop/fs/AbstractFileSystem.java +++ b/src/java/org/apache/hadoop/fs/AbstractFileSystem.java @@ -603,6 +603,14 @@ public abstract class AbstractFileSystem { dstStatus = null; } if (dstStatus != null) { + if (dst.equals(src)) { + throw new FileAlreadyExistsException( + "The source "+src+" and destination "+dst+" are the same"); + } + if (srcStatus.isSymlink() && dst.equals(srcStatus.getSymlink())) { + throw new FileAlreadyExistsException( + "Cannot rename symlink "+src+" to its target "+dst); + } if (srcStatus.isDir() != dstStatus.isDir()) { throw new IOException("Source " + src + " Destination " + dst + " both should be either file or directory"); diff --git a/src/test/core/org/apache/hadoop/fs/FileContextMainOperationsBaseTest.java b/src/test/core/org/apache/hadoop/fs/FileContextMainOperationsBaseTest.java index 0a6ab8140f6..c21c19a7442 100644 --- a/src/test/core/org/apache/hadoop/fs/FileContextMainOperationsBaseTest.java +++ b/src/test/core/org/apache/hadoop/fs/FileContextMainOperationsBaseTest.java @@ -824,6 +824,26 @@ public abstract class FileContextMainOperationsBaseTest { rename(src, dst, true, false, true, Rename.OVERWRITE); } + @Test + public void testRenameFileToItself() throws Exception { + if (!renameSupported()) return; + Path src = getTestRootPath(fc, "test/hadoop/file"); + createFile(src); + try { + rename(src, src, false, true, false, Rename.NONE); + Assert.fail("Renamed file to itself"); + } catch (IOException e) { + Assert.assertTrue(unwrapException(e) instanceof FileAlreadyExistsException); + } + // Also fails with overwrite + try { + rename(src, src, false, true, false, Rename.OVERWRITE); + Assert.fail("Renamed file to itself"); + } catch (IOException e) { + Assert.assertTrue(unwrapException(e) instanceof FileAlreadyExistsException); + } + } + @Test public void testRenameFileAsExistingFile() throws Exception { if (!renameSupported()) return; @@ -869,6 +889,26 @@ public abstract class FileContextMainOperationsBaseTest { } } + @Test + public void testRenameDirectoryToItself() throws Exception { + if (!renameSupported()) return; + Path src = getTestRootPath(fc, "test/hadoop/dir"); + fc.mkdir(src, FileContext.DEFAULT_PERM, true); + try { + rename(src, src, false, true, false, Rename.NONE); + Assert.fail("Renamed directory to itself"); + } catch (IOException e) { + Assert.assertTrue(unwrapException(e) instanceof FileAlreadyExistsException); + } + // Also fails with overwrite + try { + rename(src, src, false, true, false, Rename.OVERWRITE); + Assert.fail("Renamed directory to itself"); + } catch (IOException e) { + Assert.assertTrue(unwrapException(e) instanceof FileAlreadyExistsException); + } + } + @Test public void testRenameDirectoryToNonExistentParent() throws Exception { if (!renameSupported()) return; diff --git a/src/test/core/org/apache/hadoop/fs/FileContextSymlinkBaseTest.java b/src/test/core/org/apache/hadoop/fs/FileContextSymlinkBaseTest.java index 226bb4aa04d..de1604533dc 100644 --- a/src/test/core/org/apache/hadoop/fs/FileContextSymlinkBaseTest.java +++ b/src/test/core/org/apache/hadoop/fs/FileContextSymlinkBaseTest.java @@ -51,6 +51,10 @@ public abstract class FileContextSymlinkBaseTest { abstract protected String testBaseDir2(); abstract protected URI testURI(); + protected IOException unwrapException(IOException e) { + return e; + } + protected static void createAndWriteFile(FileContext fc, Path p) throws IOException { FSDataOutputStream out; @@ -766,7 +770,25 @@ public abstract class FileContextSymlinkBaseTest { assertFalse(fc.exists(file)); assertTrue(fc.exists(fileNewViaLink)); } - + + @Test + /** Rename a symlink to itself */ + public void testRenameSymlinkToItself() throws IOException { + Path link = new Path(testBaseDir1(), "linkToFile1"); + fc.createSymlink(new Path("/doestNotExist"), link, false); + try { + fc.rename(link, link); + } catch (IOException e) { + assertTrue(unwrapException(e) instanceof FileAlreadyExistsException); + } + // Fails with overwrite as well + try { + fc.rename(link, link, Rename.OVERWRITE); + } catch (IOException e) { + assertTrue(unwrapException(e) instanceof FileAlreadyExistsException); + } + } + @Test /** Rename a symlink */ public void testRenameSymlink() throws IOException { @@ -786,8 +808,84 @@ public abstract class FileContextSymlinkBaseTest { } catch (IOException x) { // Expected } - } - + } + + @Test + /** Rename a symlink to the file it links to */ + public void testRenameSymlinkToFileItLinksTo() throws IOException { + /* NB: The rename is not atomic, so file is deleted before renaming + * linkToFile. In this interval linkToFile is dangling and local file + * system does not handle dangling links because File.exists returns + * false for dangling links. */ + if ("file".equals(getScheme())) { + return; + } + Path file = new Path(testBaseDir1(), "file"); + Path link = new Path(testBaseDir1(), "linkToFile"); + createAndWriteFile(file); + fc.createSymlink(file, link, false); + try { + fc.rename(link, file); + fail("Renamed symlink to its target"); + } catch (IOException e) { + assertTrue(unwrapException(e) instanceof FileAlreadyExistsException); + } + // Check the rename didn't happen + assertTrue(fc.isFile(file)); + assertTrue(fc.exists(link)); + assertTrue(fc.getFileLinkStatus(link).isSymlink()); + assertEquals(file, fc.getLinkTarget(link)); + try { + fc.rename(link, file, Rename.OVERWRITE); + fail("Renamed symlink to its target"); + } catch (IOException e) { + assertTrue(unwrapException(e) instanceof FileAlreadyExistsException); + } + // Check the rename didn't happen + assertTrue(fc.isFile(file)); + assertTrue(fc.exists(link)); + assertTrue(fc.getFileLinkStatus(link).isSymlink()); + assertEquals(file, fc.getLinkTarget(link)); + } + + @Test + /** Rename a symlink to the directory it links to */ + public void testRenameSymlinkToDirItLinksTo() throws IOException { + /* NB: The rename is not atomic, so dir is deleted before renaming + * linkToFile. In this interval linkToFile is dangling and local file + * system does not handle dangling links because File.exists returns + * false for dangling links. */ + if ("file".equals(getScheme())) { + return; + } + Path dir = new Path(testBaseDir1(), "dir"); + Path link = new Path(testBaseDir1(), "linkToDir"); + fc.mkdir(dir, FileContext.DEFAULT_PERM, false); + fc.createSymlink(dir, link, false); + try { + fc.rename(link, dir); + fail("Renamed symlink to its target"); + } catch (IOException e) { + assertTrue(unwrapException(e) instanceof FileAlreadyExistsException); + } + // Check the rename didn't happen + assertTrue(fc.isDirectory(dir)); + assertTrue(fc.exists(link)); + assertTrue(fc.getFileLinkStatus(link).isSymlink()); + assertEquals(dir, fc.getLinkTarget(link)); + try { + fc.rename(link, dir, Rename.OVERWRITE); + fail("Renamed symlink to its target"); + } catch (IOException e) { + assertTrue(unwrapException(e) instanceof FileAlreadyExistsException); + } + // Check the rename didn't happen + assertTrue(fc.isDirectory(dir)); + assertTrue(fc.exists(link)); + assertTrue(fc.getFileLinkStatus(link).isSymlink()); + assertEquals(dir, fc.getLinkTarget(link)); + } + @Test /** Test renaming symlink target */ public void testMoveLinkTarget() throws IOException {