Uploaded image for project: 'Thrift'
  1. Thrift
  2. THRIFT-5310

Ruby BinaryProtocol has invalid range checks for byte and i64

    XMLWordPrintableJSON

    Details

    • Type: Bug
    • Status: Open
    • Priority: Major
    • Resolution: Unresolved
    • Affects Version/s: 0.13.0
    • Fix Version/s: None
    • Component/s: Ruby - Library
    • Labels:
      None

      Description

      Hello,

      The range checks in https://github.com/apache/thrift/blob/master/lib/rb/lib/thrift/protocol/binary_protocol.rb are wrong.

      Right now, if for example, someone accidentally writes an unsigned byte like 255, it would read as -1 on the other end of the line.

       

      For write_byte, it should be:

       byte < -2**7 || byte >= 2**7 

      For write_i64, it should be:

       i64 < -2**63 || i64 >= 2**63 

      And i just noticed that for write_i16 there is no range check at all.

       

      It is probably also better to put the calculated min/max values into consts. I did a quick benchmark:

                                 user     system      total        real
      with const:            0.526762   0.000562   0.527324 (  0.528134)
      with calculation:      2.384387   0.002364   2.386751 (  2.389461)

      Here is the code:

      require 'benchmark'
      
      MIN_I64 = -2**63
      MAX_I64 = 2**63 - 1
      def check_with_const(i64)
        i64 < MIN_I64 || i64 > MAX_I64
      end
      
      def check_with_calc(i64)
        i64 < -2**63 || i64 >= 2**63
      end
      
      n = 5_000_000
      Benchmark.bm(20) do |x|
        x.report('with const:') { (1..n).each { |i|; check_with_const(i) } }
        x.report('with calculation:') { (1..n).each { |i|; check_with_calc(i) } }
      end
      

       

      If PR's are welcome i'm more than happy to provide one.

       

      Best regards,
      Eric

       

       

        Attachments

          Activity

            People

            • Assignee:
              Unassigned
              Reporter:
              Eric Koch Eric-Christian Koch
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated: