Hadoop Common
  1. Hadoop Common
  2. HADOOP-10150 Hadoop cryptographic file system
  3. HADOOP-10662

NullPointerException in CryptoInputStream while wrapped stream is not ByteBufferReadable. Add tests using normal stream.

    Details

      Description

      NullPointerException in CryptoInputStream while wrapped stream is not ByteBufferReadable.
      Add tests for crypto streams using normal stream which does not support the additional interfaces that the Hadoop FileSystem streams implement (Seekable, PositionedReadable, ByteBufferReadable, HasFileDescriptor, CanSetDropBehind, CanSetReadahead, HasEnhancedByteBufferAccess, Syncable, CanSetDropBehind).

        Activity

        Hide
        Yi Liu added a comment -

        The patch includes the fix and tests.

        Show
        Yi Liu added a comment - The patch includes the fix and tests.
        Hide
        Alejandro Abdelnur added a comment - - edited

        Yi, the current logic, while correct seems a bit too complex. Under what circumstances you would have an InputStream that implements ByteBufferReadable but does not support the read(ByteBuffer) operation? It seems to me that would be an incorrect implementation of such interface, no?

              if (usingByteBufferRead == null) {
                if (in instanceof ByteBufferReadable) {
                  try {
                    n = ((ByteBufferReadable) in).read(inBuffer);
                    usingByteBufferRead = Boolean.TRUE;
                  } catch (UnsupportedOperationException e) {
                    usingByteBufferRead = Boolean.FALSE;
                  }
                } else {
                  usingByteBufferRead = Boolean.FALSE;
                }
                if (!usingByteBufferRead) {
                  n = readFromUnderlyingStream(inBuffer);
                }
              } else {
                if (usingByteBufferRead) {
                  n = ((ByteBufferReadable) in).read(inBuffer);
                } else {
                  n = readFromUnderlyingStream(inBuffer);
                }
              }
        

        If we assume that any InputStream implementing ByteBufferReadable supports the read(ByteBuffer) then the above code could be simplified to:

              if (usingByteBufferRead == null) {
                usingByteBufferRead = (in instanceof ByteBufferReadable);
              }
              if (usingByteBufferRead) {
                n = ((ByteBufferReadable) in).read(inBuffer);
              } else {
                n = readFromUnderlyingStream(inBuffer);
              }
        

        Or am I missing something obvious?

        Show
        Alejandro Abdelnur added a comment - - edited Yi, the current logic, while correct seems a bit too complex. Under what circumstances you would have an InputStream that implements ByteBufferReadable but does not support the read(ByteBuffer) operation? It seems to me that would be an incorrect implementation of such interface, no? if (usingByteBufferRead == null ) { if (in instanceof ByteBufferReadable) { try { n = ((ByteBufferReadable) in).read(inBuffer); usingByteBufferRead = Boolean .TRUE; } catch (UnsupportedOperationException e) { usingByteBufferRead = Boolean .FALSE; } } else { usingByteBufferRead = Boolean .FALSE; } if (!usingByteBufferRead) { n = readFromUnderlyingStream(inBuffer); } } else { if (usingByteBufferRead) { n = ((ByteBufferReadable) in).read(inBuffer); } else { n = readFromUnderlyingStream(inBuffer); } } If we assume that any InputStream implementing ByteBufferReadable supports the read(ByteBuffer) then the above code could be simplified to: if (usingByteBufferRead == null ) { usingByteBufferRead = (in instanceof ByteBufferReadable); } if (usingByteBufferRead) { n = ((ByteBufferReadable) in).read(inBuffer); } else { n = readFromUnderlyingStream(inBuffer); } Or am I missing something obvious?
        Hide
        Yi Liu added a comment -

        Hi Alejandro, Thanks for review and investigation on this. Actually in the earliest version of my local patch, I did as your suggestion. But it had issue and I found it in test cases.

        Under what circumstances you would have an InputStream that implements ByteBufferReadable but does not support the read(ByteBuffer) operation?

        In Hadoop, there are many layers of InputStream. Even some InputStream implements ByteBufferReadable, but whether it support read(ByteBuffer) is decided by its wrapped underlying streams.

        I use FSDataInputStream as example, absolutely it implements ByteBufferReadable, but it's a high layer wrapper, that doesn't mean it supports read(ByteBuffer), let’s look at its implementation:

        @Override
        public int read(ByteBuffer buf) throws IOException {
          if (in instanceof ByteBufferReadable) {
            return ((ByteBufferReadable)in).read(buf);
          }
        
          throw new UnsupportedOperationException("Byte-buffer read unsupported by input stream");
        }
        

        We can see it depends on the implementation of its wrapped input stream.

        In our testcases and also test cases in HDFS-6405, we cover different environments(HDFS, local FS) and different streams type. We can find this scenario in TestCryptoStreamsForLocalFS.

        Show
        Yi Liu added a comment - Hi Alejandro, Thanks for review and investigation on this. Actually in the earliest version of my local patch, I did as your suggestion. But it had issue and I found it in test cases. Under what circumstances you would have an InputStream that implements ByteBufferReadable but does not support the read(ByteBuffer) operation? In Hadoop, there are many layers of InputStream. Even some InputStream implements ByteBufferReadable , but whether it support read(ByteBuffer) is decided by its wrapped underlying streams. I use FSDataInputStream as example, absolutely it implements ByteBufferReadable , but it's a high layer wrapper, that doesn't mean it supports read(ByteBuffer) , let’s look at its implementation: @Override public int read(ByteBuffer buf) throws IOException { if (in instanceof ByteBufferReadable) { return ((ByteBufferReadable)in).read(buf); } throw new UnsupportedOperationException( " Byte -buffer read unsupported by input stream" ); } We can see it depends on the implementation of its wrapped input stream. In our testcases and also test cases in HDFS-6405 , we cover different environments(HDFS, local FS) and different streams type. We can find this scenario in TestCryptoStreamsForLocalFS .
        Hide
        Alejandro Abdelnur added a comment -

        got it, thx for looking at it. +1

        Show
        Alejandro Abdelnur added a comment - got it, thx for looking at it. +1
        Hide
        Yi Liu added a comment -

        Thanks Alejandro
        I also had this assumption as you, and found an issue in test cases.
        I will commit it to branch later.

        Show
        Yi Liu added a comment - Thanks Alejandro I also had this assumption as you, and found an issue in test cases. I will commit it to branch later.
        Hide
        Yi Liu added a comment -

        Committed to branch.

        Show
        Yi Liu added a comment - Committed to branch.

          People

          • Assignee:
            Yi Liu
            Reporter:
            Yi Liu
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development