Uploaded image for project: 'Flink'
  1. Flink
  2. FLINK-6044

TypeSerializerSerializationProxy.read() doesn't verify the read buffer length

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 1.2.0
    • Fix Version/s: 1.3.0
    • Labels:
      None
    • Environment:

      Ubuntu server 12.04.5 64 bit
      java version "1.8.0_111"
      Java(TM) SE Runtime Environment (build 1.8.0_111-b14)
      Java HotSpot(TM) 64-Bit Server VM (build 25.111-b14, mixed mode)

      Description

      The read() method of TypeSerializerSerializationProxy creates a buffers and tries to fill it by calling the read() method of the given DataInputView, but never checks the return value. The actual size read from the stream might be smaller than the buffer size, and the rest of the buffer is filled with zeroes, causing the deserialization to fail.
      It happened to me using a RocksDB state backend backed by S3. The setup was done according to https://ci.apache.org/projects/flink/flink-docs-release-1.2/setup/aws.html#s3-simple-storage-service and everything worked correctly until I upgraded to Flink 1.2.0.

        Issue Links

          Activity

          Hide
          StephanEwen Stephan Ewen added a comment -

          The read() / readFully() issue has occurred before, it will most likely occur again.

          A simple thing we can do is "forbid" that method in the same way as String/Byte conversions without a charset.
          Take a look at the CheckForbiddenMethodsUsage test in the manual tests. We can add such as test for the read() method.

          We only need to remember executing the manual tests on every release.

          Show
          StephanEwen Stephan Ewen added a comment - The read() / readFully() issue has occurred before, it will most likely occur again. A simple thing we can do is "forbid" that method in the same way as String/Byte conversions without a charset. Take a look at the CheckForbiddenMethodsUsage test in the manual tests. We can add such as test for the read() method. We only need to remember executing the manual tests on every release.
          Hide
          StephanEwen Stephan Ewen added a comment -

          Stefan Richter I think we should try and safeguard Flink against running into the same bug again.

          Show
          StephanEwen Stephan Ewen added a comment - Stefan Richter I think we should try and safeguard Flink against running into the same bug again.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StefanRRichter closed the pull request at:

          https://github.com/apache/flink/pull/3533

          Show
          githubbot ASF GitHub Bot added a comment - Github user StefanRRichter closed the pull request at: https://github.com/apache/flink/pull/3533
          Hide
          srichter Stefan Richter added a comment -

          Fixed in cbaf4e5 (master)

          Show
          srichter Stefan Richter added a comment - Fixed in cbaf4e5 (master)
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StefanRRichter commented on the issue:

          https://github.com/apache/flink/pull/3533

          Thanks for the review @tillrohrmann. Merging this.

          Show
          githubbot ASF GitHub Bot added a comment - Github user StefanRRichter commented on the issue: https://github.com/apache/flink/pull/3533 Thanks for the review @tillrohrmann. Merging this.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user tillrohrmann commented on the issue:

          https://github.com/apache/flink/pull/3533

          Thanks for your contribution @StefanRRichter. Changes look good to me. +1 for merging.

          Show
          githubbot ASF GitHub Bot added a comment - Github user tillrohrmann commented on the issue: https://github.com/apache/flink/pull/3533 Thanks for your contribution @StefanRRichter. Changes look good to me. +1 for merging.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StefanRRichter commented on the issue:

          https://github.com/apache/flink/pull/3533

          CC @uce @tillrohrmann

          Show
          githubbot ASF GitHub Bot added a comment - Github user StefanRRichter commented on the issue: https://github.com/apache/flink/pull/3533 CC @uce @tillrohrmann
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user StefanRRichter opened a pull request:

          https://github.com/apache/flink/pull/3533

          FLINK-6044 Replace calls to InputStream#read(...) with the indended…

          This PR fixes FLINK-6044. On top of that, I searched through the code for calls to `InputStream#read(...)` that ignore their return value and replaced them with the intended `InputStream#readFully(...)` calls.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/StefanRRichter/flink FLINK-6044-readFully

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/flink/pull/3533.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #3533


          commit 342144a967c5d93ba87f1d2850b88dad7c796f81
          Author: Stefan Richter <s.richter@data-artisans.com>
          Date: 2017-03-14T13:45:00Z

          FLINK-6044 Replace calls to InputStream#read(...) with the indended InputStream#readFully(...)


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user StefanRRichter opened a pull request: https://github.com/apache/flink/pull/3533 FLINK-6044 Replace calls to InputStream#read(...) with the indended… This PR fixes FLINK-6044 . On top of that, I searched through the code for calls to `InputStream#read(...)` that ignore their return value and replaced them with the intended `InputStream#readFully(...)` calls. You can merge this pull request into a Git repository by running: $ git pull https://github.com/StefanRRichter/flink FLINK-6044 -readFully Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/3533.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #3533 commit 342144a967c5d93ba87f1d2850b88dad7c796f81 Author: Stefan Richter <s.richter@data-artisans.com> Date: 2017-03-14T13:45:00Z FLINK-6044 Replace calls to InputStream#read(...) with the indended InputStream#readFully(...)
          Hide
          srichter Stefan Richter added a comment -

          Thanks for reporting this. The read(...) is intended to be a readFully(...). I will fix this.

          Show
          srichter Stefan Richter added a comment - Thanks for reporting this. The read(...) is intended to be a readFully(...) . I will fix this.

            People

            • Assignee:
              srichter Stefan Richter
              Reporter:
              avihai.berkovitz@microsoft.com Avihai Berkovitz
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development