Hadoop Common
  1. Hadoop Common
  2. HADOOP-8179

risk of NPE in CopyCommands processArguments()

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.23.2, 0.24.0
    • Fix Version/s: 0.23.3, 2.0.2-alpha
    • Component/s: fs
    • Labels:
      None

      Description

      My IDE is warning me that the is.close() method will NPE if the is = src.fs.open(src.path); call raises an exception, which could happen if the source path could not be opened. There should be an if (is!=null) wrapper

      1. HADOOP-8179.patch
        0.8 kB
        Daryn Sharp
      2. HADOOP-8179.patch
        0.9 kB
        Steve Loughran

        Issue Links

          Activity

          Steve Loughran created issue -
          Hide
          Daryn Sharp added a comment -

          Which file/line is making your IDE unhappy? I don't see the exact line, but I think you are referring to the copymerge command? I see that it's opening the file inside of a try block with a finally clause that closes the stream. It looks like the open - the first line of the try - should be moved to line before the try.

          Show
          Daryn Sharp added a comment - Which file/line is making your IDE unhappy? I don't see the exact line, but I think you are referring to the copymerge command? I see that it's opening the file inside of a try block with a finally clause that closes the stream. It looks like the open - the first line of the try - should be moved to line before the try.
          Hide
          Steve Loughran added a comment -

          Sorry, I should have given more detail.
          yes, that's what it's complaining about, and yes, moving it up would be the other solution.

          If done rigorously a test ought to go with the patch too, but it is a very small change...

          Show
          Steve Loughran added a comment - Sorry, I should have given more detail. yes, that's what it's complaining about, and yes, moving it up would be the other solution. If done rigorously a test ought to go with the patch too, but it is a very small change...
          Hide
          Steve Loughran added a comment -

          Moved the is=src.fs.open() out the try/catch block

          Show
          Steve Loughran added a comment - Moved the is=src.fs.open() out the try/catch block
          Steve Loughran made changes -
          Field Original Value New Value
          Attachment HADOOP-8179.patch [ 12518701 ]
          Hide
          Steve Loughran added a comment -

          Patch as suggested by Daryn, pull the assignment up. No tests.

          Show
          Steve Loughran added a comment - Patch as suggested by Daryn, pull the assignment up. No tests.
          Steve Loughran made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Affects Version/s 0.23.2 [ 12319855 ]
          Daryn Sharp made changes -
          Link This issue is related to HADOOP-8140 [ HADOOP-8140 ]
          Hide
          Daryn Sharp added a comment -

          +1 Perfect. I was about to throw up the same patch.

          Show
          Daryn Sharp added a comment - +1 Perfect. I was about to throw up the same patch.
          Hide
          Daryn Sharp added a comment -

          I'll see if I can find some of the tests I wrote for getmerge but didn't post because the hdfs cli tests were broken at the time.

          Show
          Daryn Sharp added a comment - I'll see if I can find some of the tests I wrote for getmerge but didn't post because the hdfs cli tests were broken at the time.
          Hide
          Daryn Sharp added a comment -

          Didn't find them. I tried to write a test, but I don't think it's (easily) possible. I tried inducing an open failure via permissions. Even setting the perms on a file to 000, the test user is considered a superuser to the mini cluster, so it walks right through the permissions...

          Show
          Daryn Sharp added a comment - Didn't find them. I tried to write a test, but I don't think it's (easily) possible. I tried inducing an open failure via permissions. Even setting the perms on a file to 000, the test user is considered a superuser to the mini cluster, so it walks right through the permissions...
          Hide
          Daryn Sharp added a comment -

          On second thought, do you think it would be better to scope the var and do:

                 try {
          -        FSDataInputStream in = null;
                   for (PathData src : srcs) {
          +          FSDataInputStream in = src.fs.open(src.path);
                     try {
          -            in = src.fs.open(src.path);
                       IOUtils.copyBytes(in, out, getConf(), false);
          
          Show
          Daryn Sharp added a comment - On second thought, do you think it would be better to scope the var and do: try { - FSDataInputStream in = null ; for (PathData src : srcs) { + FSDataInputStream in = src.fs.open(src.path); try { - in = src.fs.open(src.path); IOUtils.copyBytes(in, out, getConf(), false );
          Hide
          Daryn Sharp added a comment -

          My previous suggestion

          Show
          Daryn Sharp added a comment - My previous suggestion
          Daryn Sharp made changes -
          Attachment HADOOP-8179.patch [ 12518735 ]
          Daryn Sharp made changes -
          Assignee Daryn Sharp [ daryn ]
          Hide
          Hadoop QA added a comment -

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

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +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 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

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

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

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/723//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/723//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/12518735/HADOOP-8179.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +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 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in . +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/723//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/723//console This message is automatically generated.
          Hide
          Daryn Sharp added a comment -

          Regarding no test, I was able to test locally by: starting a mini-cluster, creating a file with insufficient privs for different user (non-superuser) to access, then tried to access the file with that different user. I saw the NPE before, but not after, the patch.

          Show
          Daryn Sharp added a comment - Regarding no test, I was able to test locally by: starting a mini-cluster, creating a file with insufficient privs for different user (non-superuser) to access, then tried to access the file with that different user. I saw the NPE before, but not after, the patch.
          Hide
          Uma Maheswara Rao G added a comment -

          Hi Daryn, Patch mostly looks good. I have other suggestion to do.

                       if (delimiter != null) {
                         out.write(delimiter.getBytes("UTF-8"));
                       }
          -          } finally {
                       in.close();
          -          }
          +	    in = null;
          +	  } finally {
          +	    IOUtils.closeStream(in);
          +	  }
                   }
          +        out.close();
          +	 out = null;
                 } finally {
          -        out.close();
          +	 IOUtils.closeStream(out);
                 }      
               }
          
          

          closing stream in finally may mask the root exceptions if any exception in finally.
          So, i would prefer this pattern to close the streams. Let's close the stream in try itself and nullify if that is success. We can use IOUtils.closeStream in finally. It won't try to close the stream if that is null.

          Show
          Uma Maheswara Rao G added a comment - Hi Daryn, Patch mostly looks good. I have other suggestion to do. if (delimiter != null ) { out.write(delimiter.getBytes( "UTF-8" )); } - } finally { in.close(); - } + in = null ; + } finally { + IOUtils.closeStream(in); + } } + out.close(); + out = null ; } finally { - out.close(); + IOUtils.closeStream(out); } } closing stream in finally may mask the root exceptions if any exception in finally. So, i would prefer this pattern to close the streams. Let's close the stream in try itself and nullify if that is success. We can use IOUtils.closeStream in finally. It won't try to close the stream if that is null.
          Hide
          Daryn Sharp added a comment -

          Good eye, but I'm actually fixing the close issue on a separate jira. Just an aside, using IOUtils to close the output stream is inappropriate because it will mask exceptions that may be related to finalizing the block.

          Show
          Daryn Sharp added a comment - Good eye, but I'm actually fixing the close issue on a separate jira. Just an aside, using IOUtils to close the output stream is inappropriate because it will mask exceptions that may be related to finalizing the block.
          Hide
          Daryn Sharp added a comment -

          On further thought, you are right that it may be useful to conditionally use IOStreams.close on the output stream based on whether an exception has already thrown.

          Show
          Daryn Sharp added a comment - On further thought, you are right that it may be useful to conditionally use IOStreams.close on the output stream based on whether an exception has already thrown.
          Hide
          Robert Joseph Evans added a comment -

          The patch looks simple enough that no unit tests are needed and it fixes the observed issue. +1

          Show
          Robert Joseph Evans added a comment - The patch looks simple enough that no unit tests are needed and it fixes the observed issue. +1
          Hide
          Robert Joseph Evans added a comment -

          Thanks Daryn, I just put this into trunk, branch-2, and branch-0.23

          Show
          Robert Joseph Evans added a comment - Thanks Daryn, I just put this into trunk, branch-2, and branch-0.23
          Robert Joseph Evans made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Fix Version/s 0.23.3 [ 12320059 ]
          Fix Version/s 2.0.0 [ 12320352 ]
          Resolution Fixed [ 1 ]
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #2072 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2072/)
          HADOOP-8179. risk of NPE in CopyCommands processArguments() (Daryn Sharp via bobby) (Revision 1309572)

          Result = SUCCESS
          bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1309572
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #2072 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2072/ ) HADOOP-8179 . risk of NPE in CopyCommands processArguments() (Daryn Sharp via bobby) (Revision 1309572) Result = SUCCESS bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1309572 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #1997 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/1997/)
          HADOOP-8179. risk of NPE in CopyCommands processArguments() (Daryn Sharp via bobby) (Revision 1309572)

          Result = SUCCESS
          bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1309572
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #1997 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/1997/ ) HADOOP-8179 . risk of NPE in CopyCommands processArguments() (Daryn Sharp via bobby) (Revision 1309572) Result = SUCCESS bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1309572 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #2009 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2009/)
          HADOOP-8179. risk of NPE in CopyCommands processArguments() (Daryn Sharp via bobby) (Revision 1309572)

          Result = SUCCESS
          bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1309572
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #2009 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2009/ ) HADOOP-8179 . risk of NPE in CopyCommands processArguments() (Daryn Sharp via bobby) (Revision 1309572) Result = SUCCESS bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1309572 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-0.23-Build #219 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Build/219/)
          svn merge -c 1309572 from trunk. FIXES HADOOP-8179. risk of NPE in CopyCommands processArguments() (Daryn Sharp via bobby) (Revision 1309574)

          Result = SUCCESS
          bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1309574
          Files :

          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-0.23-Build #219 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Build/219/ ) svn merge -c 1309572 from trunk. FIXES HADOOP-8179 . risk of NPE in CopyCommands processArguments() (Daryn Sharp via bobby) (Revision 1309574) Result = SUCCESS bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1309574 Files : /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #1006 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1006/)
          HADOOP-8179. risk of NPE in CopyCommands processArguments() (Daryn Sharp via bobby) (Revision 1309572)

          Result = FAILURE
          bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1309572
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1006 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1006/ ) HADOOP-8179 . risk of NPE in CopyCommands processArguments() (Daryn Sharp via bobby) (Revision 1309572) Result = FAILURE bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1309572 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1041 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1041/)
          HADOOP-8179. risk of NPE in CopyCommands processArguments() (Daryn Sharp via bobby) (Revision 1309572)

          Result = SUCCESS
          bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1309572
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1041 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1041/ ) HADOOP-8179 . risk of NPE in CopyCommands processArguments() (Daryn Sharp via bobby) (Revision 1309572) Result = SUCCESS bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1309572 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java
          Arun C Murthy made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Arun C Murthy made changes -
          Fix Version/s 2.0.2-alpha [ 12322473 ]
          Fix Version/s 2.0.0-alpha [ 12320352 ]
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Patch Available Patch Available
          5h 34m 1 Steve Loughran 16/Mar/12 16:51
          Patch Available Patch Available Resolved Resolved
          19d 3h 4m 1 Robert Joseph Evans 04/Apr/12 20:56
          Resolved Resolved Closed Closed
          49d 19m 1 Arun C Murthy 23/May/12 21:15

            People

            • Assignee:
              Daryn Sharp
              Reporter:
              Steve Loughran
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development