Hadoop Common
  1. Hadoop Common
  2. HADOOP-4271

Bug in FSInputChecker makes it possible to read from an invalid buffer

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 0.16.0
    • Fix Version/s: 0.17.3, 0.18.2
    • Component/s: fs
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      Checksum input stream can sometimes return invalid data to the user.

      Description

      Bug in FSInputChecker makes it possible to read from an invalid buffer. The buffer in FSInputChecker becomes invalid when readChecksumChunk is used to read a chunk to a user buffer directly. Currently, it's not marked as invalid in this case and may be read subsequently.

      1. HADOOP-4277-branch-18-correction.patch
        1 kB
        Raghu Angadi
      2. hadoop-4271.patch
        3 kB
        Raghu Angadi
      3. hadoop-4271-branch-18.patch
        3 kB
        Raghu Angadi
      4. hadoop-4271-branch-18.patch
        6 kB
        Raghu Angadi
      5. hadoop-4271-branch-17.patch
        3 kB
        Raghu Angadi
      6. hadoop-4271.patch
        3 kB
        Ning Li
      7. hadoop-4271.patch
        5 kB
        Ning Li
      8. .data_testFSInputChecker.crc
        0.0 kB
        Ning Li
      9. data_testFSInputChecker
        1 kB
        Ning Li
      10. hadoop-4271.patch
        8 kB
        Ning Li

        Issue Links

          Activity

          Hide
          Ning Li added a comment -

          Attached is the patch for this issue. A test is added to verify the fix. Two data files used by the test is also included. All tests pass with the patch.

          Show
          Ning Li added a comment - Attached is the patch for this issue. A test is added to verify the fix. Two data files used by the test is also included. All tests pass with the patch.
          Hide
          Raghu Angadi added a comment -

          Good catch. This can happen when seek() is used. A few things:

          • I think (more) correct fix is to reset 'pos' and 'count' before calling {{ readChecksumChunk()}}.. since once it is called, buffer is no longer valid (say there is some exception and user seeks again).
          • You can generate 1500 bytes of random data in the test itself rather than requiring two extra files to be committed.
          • Currently 'runTest()' fails only with checksum disabled. Same bug exists with checksum enabled as well, which is the common case. Can you change the test so that it fails with checksum enabled as well?
          • There is already a TestFSInputChecksum.java in hdfs. Can you add this case there?
          • Minor : using IOUtils.readFully() will reduce the code for the test.
          Show
          Raghu Angadi added a comment - Good catch. This can happen when seek() is used. A few things: I think (more) correct fix is to reset 'pos' and 'count' before calling {{ readChecksumChunk()}}.. since once it is called, buffer is no longer valid (say there is some exception and user seeks again). You can generate 1500 bytes of random data in the test itself rather than requiring two extra files to be committed. Currently 'runTest()' fails only with checksum disabled. Same bug exists with checksum enabled as well, which is the common case. Can you change the test so that it fails with checksum enabled as well? There is already a TestFSInputChecksum.java in hdfs. Can you add this case there? Minor : using IOUtils.readFully() will reduce the code for the test.
          Hide
          Raghu Angadi added a comment -

          Marking this critical since wrong data returned is a serious bug.

          Show
          Raghu Angadi added a comment - Marking this critical since wrong data returned is a serious bug.
          Hide
          Ning Li added a comment -

          I will address the comments in the next patch.

          > There is already a TestFSInputChecksum.java in hdfs. Can you add this case there?

          I see a TestFSInputChecker.java in hdfs. I'll merge the two. But should I move it to fs? Since everything tested is in fs?

          Show
          Ning Li added a comment - I will address the comments in the next patch. > There is already a TestFSInputChecksum.java in hdfs. Can you add this case there? I see a TestFSInputChecker.java in hdfs. I'll merge the two. But should I move it to fs? Since everything tested is in fs?
          Hide
          Ning Li added a comment -

          This patch addresses all the comments. I leave TestFSInputChecker.java in hdfs since it tests on MiniDFSCluster as well. The price is that I have to make ChecksumFileSystem.open(Path f, int bufferSize, boolean verifyChecksum) a public method.

          Show
          Ning Li added a comment - This patch addresses all the comments. I leave TestFSInputChecker.java in hdfs since it tests on MiniDFSCluster as well. The price is that I have to make ChecksumFileSystem.open(Path f, int bufferSize, boolean verifyChecksum) a public method.
          Hide
          Raghu Angadi added a comment -

          Thanks Ning.

          > The price is that I have to make ChecksumFileSystem.open(Path f, int bufferSize, boolean verifyChecksum) a public method.

          You really don't need change ChecksumFileSystem at all. You don't need to test with verifyChecksum off I think. It will make the patch much simpler.

          Minor: for the same reason, it might be better to set count to zero in fill() as well. We could remove resetting these inside readChecksumChunk() (in case of checksum exception).

          Show
          Raghu Angadi added a comment - Thanks Ning. > The price is that I have to make ChecksumFileSystem.open(Path f, int bufferSize, boolean verifyChecksum) a public method. You really don't need change ChecksumFileSystem at all. You don't need to test with verifyChecksum off I think. It will make the patch much simpler. Minor: for the same reason, it might be better to set count to zero in fill() as well. We could remove resetting these inside readChecksumChunk() (in case of checksum exception).
          Hide
          Ning Li added a comment -

          > You really don't need change ChecksumFileSystem at all. You don't need to test with verifyChecksum off I think. It will make the patch much simpler.

          Before the patch, the default of verifyChecksum in FSInputChecker is false. I made the change in order to test with verifyChecksum being true.

          Show
          Ning Li added a comment - > You really don't need change ChecksumFileSystem at all. You don't need to test with verifyChecksum off I think. It will make the patch much simpler. Before the patch, the default of verifyChecksum in FSInputChecker is false. I made the change in order to test with verifyChecksum being true.
          Hide
          Ning Li added a comment -

          > > You really don't need change ChecksumFileSystem at all. You don't need to test with verifyChecksum off I think. It will make the patch much simpler.

          > Before the patch, the default of verifyChecksum in FSInputChecker is false. I made the change in order to test with verifyChecksum being true.

          Should the default of verifyChecksum be true? If so and if we only test with verifyChecksum being true, then yes, the patch can be much simpler.

          Show
          Ning Li added a comment - > > You really don't need change ChecksumFileSystem at all. You don't need to test with verifyChecksum off I think. It will make the patch much simpler. > Before the patch, the default of verifyChecksum in FSInputChecker is false. I made the change in order to test with verifyChecksum being true. Should the default of verifyChecksum be true? If so and if we only test with verifyChecksum being true, then yes, the patch can be much simpler.
          Hide
          Raghu Angadi added a comment -

          > Before the patch, the default of verifyChecksum in FSInputChecker is false.

          This is a surprise. It should be true by default. You might have found another serious bug! Let me confirm.

          Show
          Raghu Angadi added a comment - > Before the patch, the default of verifyChecksum in FSInputChecker is false. This is a surprise. It should be true by default. You might have found another serious bug! Let me confirm.
          Hide
          Raghu Angadi added a comment -

          Ning,

          I filed HADOOP-4277 for 'verifyChecksum'. Could you assign it to yourself if you don't mind fixing it? I will mark this jira dependent on it.

          Show
          Raghu Angadi added a comment - Ning, I filed HADOOP-4277 for 'verifyChecksum'. Could you assign it to yourself if you don't mind fixing it? I will mark this jira dependent on it.
          Hide
          Raghu Angadi added a comment -

          I assigned HADOOP-4277 to myself. Thanks Ning.

          Show
          Raghu Angadi added a comment - I assigned HADOOP-4277 to myself. Thanks Ning.
          Hide
          Ning Li added a comment -

          I'm confused. Do you want me to fix HADOOP-4277?

          Show
          Ning Li added a comment - I'm confused. Do you want me to fix HADOOP-4277 ?
          Hide
          Raghu Angadi added a comment -

          > I'm confused. Do you want me to fix HADOOP-4277?
          . I will fix HADOOP-4277. The patch there will conflict with the patch here. Please provide an updated patch once HADOOP-4277 is committed (today or tomorrow).

          Show
          Raghu Angadi added a comment - > I'm confused. Do you want me to fix HADOOP-4277 ? . I will fix HADOOP-4277 . The patch there will conflict with the patch here. Please provide an updated patch once HADOOP-4277 is committed (today or tomorrow).
          Hide
          Raghu Angadi added a comment -

          Ning, HADOOP-4277 is committed. Would you be able to update this patch today? You can create a patch for trunk and I will make patches for 0.17 and 0.18.

          Show
          Raghu Angadi added a comment - Ning, HADOOP-4277 is committed. Would you be able to update this patch today? You can create a patch for trunk and I will make patches for 0.17 and 0.18.
          Hide
          Ning Li added a comment -

          The updated patch with HADOOP-4277 committed.

          Show
          Ning Li added a comment - The updated patch with HADOOP-4277 committed.
          Hide
          Raghu Angadi added a comment -

          +1.

          Show
          Raghu Angadi added a comment - +1.
          Hide
          Raghu Angadi added a comment -

          0.17 and 0.18 versions of Ning's patch are attached.

          Show
          Raghu Angadi added a comment - 0.17 and 0.18 versions of Ning's patch are attached.
          Hide
          Raghu Angadi added a comment -

          test-patch output :

               [exec] +1 overall.
          
               [exec]     +1 @author.  The patch does not contain any @author tags.
          
               [exec]     +1 tests included.  The patch appears to include 3 new or modified tests.
          
               [exec]     +1 javadoc.  The javadoc tool did not generate any warning messages.
          
               [exec]     +1 javac.  The applied patch does not increase the total number of javac compiler warnings.
          
               [exec]     +1 findbugs.  The patch does not introduce any new Findbugs warnings.
          
               [exec]     +1 Eclipse classpath. The patch retains Eclipse classpath integrity.
          

          will commit the patch today.

          Show
          Raghu Angadi added a comment - test-patch output : [exec] +1 overall. [exec] +1 @author. The patch does not contain any @author tags. [exec] +1 tests included. The patch appears to include 3 new or modified tests. [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] +1 Eclipse classpath. The patch retains Eclipse classpath integrity. will commit the patch today.
          Hide
          Raghu Angadi added a comment -

          I just committed this. Thanks Ning!

          Show
          Raghu Angadi added a comment - I just committed this. Thanks Ning!
          Hide
          Hairong Kuang added a comment -

          > The buffer in FSInputChecker becomes invalid when readChecksumChunk is used to read a chunk to a user buffer directly. Currently, it's not marked as invalid in this case and may be read subsequently.

          I do not understand why this is a bug. Only when the buffer does not have any available byte, data are read to the user buffer directly. So there is no need to invalidate the buffer.

          Show
          Hairong Kuang added a comment - > The buffer in FSInputChecker becomes invalid when readChecksumChunk is used to read a chunk to a user buffer directly. Currently, it's not marked as invalid in this case and may be read subsequently. I do not understand why this is a bug. Only when the buffer does not have any available byte, data are read to the user buffer directly. So there is no need to invalidate the buffer.
          Hide
          Hairong Kuang added a comment -

          Ok I see that it effects "seek". This makes sense.

          Show
          Hairong Kuang added a comment - Ok I see that it effects "seek". This makes sense.
          Hide
          Raghu Angadi added a comment -

          Just a clarification that this does not affect HDFS reads since DFSInputStream has its own seek().

          Show
          Raghu Angadi added a comment - Just a clarification that this does not affect HDFS reads since DFSInputStream has its own seek().
          Hide
          Hudson added a comment -

          Integrated in Hadoop-trunk #620 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/620/)
          fix jira number for

          Show
          Hudson added a comment - Integrated in Hadoop-trunk #620 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/620/ ) fix jira number for

            People

            • Assignee:
              Ning Li
              Reporter:
              Ning Li
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development