Hadoop Common
  1. Hadoop Common
  2. HADOOP-5551

Namenode permits directory destruction on overwrite

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 0.19.1
    • Fix Version/s: 0.19.2, 0.20.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      The FSNamesystem's startFileInternal allows overwriting of directories. That is, if you have a directory named /foo/bar and you try to write a file named /foo/bar, the file is written and the directory disappears.

      This is most apparent for folks using libhdfs directly, as overwriting is always turned on. Therefore, if libhdfs applications do not check the existence of a directory first, then they will permit new files to destroy directories.

      1. HADOOP-5551-v2.patch
        2 kB
        Brian Bockelman
      2. HADOOP-5551-v3.patch
        2 kB
        Brian Bockelman
      3. HADOOP-5551-v4.patch
        2 kB
        Brian Bockelman

        Activity

        Hide
        Brian Bockelman added a comment -

        Suggested patch attached.

        Show
        Brian Bockelman added a comment - Suggested patch attached.
        Hide
        Konstantin Shvachko added a comment -

        Looks like your solution will work only if permission is enabled. If not it will still replace the directory by the file.
        Could you please include a test for that. You can just add a test case to TestFileCreation in order to avoid cluster startup / shutdown just for this one operation.

        Show
        Konstantin Shvachko added a comment - Looks like your solution will work only if permission is enabled. If not it will still replace the directory by the file. Could you please include a test for that. You can just add a test case to TestFileCreation in order to avoid cluster startup / shutdown just for this one operation.
        Hide
        Brian Bockelman added a comment -

        This is an improved patch - fixing the obvious thing Konstantin pointed out and contains a test case.

        Will submit it as a patch.

        Show
        Brian Bockelman added a comment - This is an improved patch - fixing the obvious thing Konstantin pointed out and contains a test case. Will submit it as a patch.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12403562/HADOOP-5551-v2.patch
        against trunk revision 758156.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 4 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/135/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/135/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/135/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/135/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12403562/HADOOP-5551-v2.patch against trunk revision 758156. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/135/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/135/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/135/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/135/console This message is automatically generated.
        Hide
        Konstantin Shvachko added a comment -

        +1 for FSNamesystem changes.

        TestCreateFile could be simplified:

        • you can assert right after creating the file, because the creation should not be successful. Listing the directory does not really add any value.
        • if by any chance the file is created you should close the output stream.
          Something like this:
                try {
                  FSDataOutputStream out = fs.create(dir1, true);
                  out.close();
                  assertTrue("Did not prevent directory from being overwritten.", false);
                } catch (IOException ie) {
                  if (!ie.getMessage().contains("already exists as a directory.")) {
                   throw ie;
                  }
                }
          
        • also you got some indentation problems in the test code (should be 2 spaces)
        • and please make sure that lines do not exceed 80 symbols.
        Show
        Konstantin Shvachko added a comment - +1 for FSNamesystem changes. TestCreateFile could be simplified: you can assert right after creating the file, because the creation should not be successful. Listing the directory does not really add any value. if by any chance the file is created you should close the output stream. Something like this: try { FSDataOutputStream out = fs.create(dir1, true ); out.close(); assertTrue( "Did not prevent directory from being overwritten." , false ); } catch (IOException ie) { if (!ie.getMessage().contains( "already exists as a directory." )) { throw ie; } } also you got some indentation problems in the test code (should be 2 spaces) and please make sure that lines do not exceed 80 symbols.
        Hide
        Brian Bockelman added a comment -

        This patch takes into consideration Konstantin's comments. Made the test case play nicer with Hadoop; same patch for FSNamesystem.

        Show
        Brian Bockelman added a comment - This patch takes into consideration Konstantin's comments. Made the test case play nicer with Hadoop; same patch for FSNamesystem.
        Hide
        Konstantin Shvachko added a comment -

        +1
        Could you please resubmit the patch so that Hudson could pick it up.

        Show
        Konstantin Shvachko added a comment - +1 Could you please resubmit the patch so that Hudson could pick it up.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12403752/HADOOP-5551-v3.patch
        against trunk revision 759398.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 4 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed core unit tests.

        -1 contrib tests. The patch failed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/75/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/75/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/75/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/75/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12403752/HADOOP-5551-v3.patch against trunk revision 759398. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/75/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/75/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/75/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/75/console This message is automatically generated.
        Hide
        Brian Bockelman added a comment -

        Shoot - accidentally negated the test condition after testing the patch locally, causing the hudson failure.

        Here's the fixed test case; I did check both testFileCreation and testFileCreationSimulated locally prior to submitting. Passes locally.

        Show
        Brian Bockelman added a comment - Shoot - accidentally negated the test condition after testing the patch locally, causing the hudson failure. Here's the fixed test case; I did check both testFileCreation and testFileCreationSimulated locally prior to submitting. Passes locally.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12404070/HADOOP-5551-v4.patch
        against trunk revision 759398.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 4 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed core unit tests.

        -1 contrib tests. The patch failed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/76/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/76/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/76/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/76/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12404070/HADOOP-5551-v4.patch against trunk revision 759398. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/76/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/76/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/76/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/76/console This message is automatically generated.
        Hide
        Brian Bockelman added a comment -

        Test failures appear to be unrelated?

        Show
        Brian Bockelman added a comment - Test failures appear to be unrelated?
        Hide
        Konstantin Shvachko added a comment -

        I just committed this. Thank you Brian.

        Show
        Konstantin Shvachko added a comment - I just committed this. Thank you Brian.
        Hide
        Hudson added a comment -
        Show
        Hudson added a comment - Integrated in Hadoop-trunk #796 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/796/ )

          People

          • Assignee:
            Brian Bockelman
            Reporter:
            Brian Bockelman
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development