[THRIFT-5310] Ruby BinaryProtocol has invalid range checks for byte and i64

• Type: Bug
• Status: Open
• Priority: Major
• Resolution: Unresolved
• Affects Version/s: 0.13.0
• Component/s: Ruby - Library
• Assignee: Unassigned
• Reporter: Eric-Christian Koch
• Created: 13/Nov/20 15:02
• Updated: 16/Nov/20 17:34

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