Eric-Christian Koch created THRIFT-5310:
-------------------------------------------
Summary: Ruby BinaryProtocol has invalid range checks for byte and
i64
Key: THRIFT-5310
URL: https://issues.apache.org/jira/browse/THRIFT-5310
Project: Thrift
Issue Type: Bug
Components: Ruby - Library
Affects Versions: 0.13.0
Reporter: Eric-Christian Koch
Hello,
The range checks in
[https://github.com/apache/thrift/blob/master/lib/rb/lib/thrift/protocol/binary_protocol.rb|https://github.com/apache/thrift/blob/master/lib/rb/lib/thrift/protocol/binary_protocol.rb#L85]
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:
{code:ruby}
byte < -2**7 || byte >= 2**7 {code}
For write_i64, it should be:
{code:ruby}
i64 < -2**63 || i64 >= 2**63 {code}
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:
{noformat}
user system total real
with const: 0.526762 0.000562 0.527324 ( 0.528134)
with calculation: 2.384387 0.002364 2.386751 ( 2.389461){noformat}
Here is the code:
{code:ruby}
require 'benchmark'
MIN_I64 = -2**63
MAX_I64 = 2**64 - 1
def check_with_const(i64)
i64 < MIN_I64 || i64 > MAX_I64
end
def check_with_calc(i64)
i64 < -2**63 || i64 >= 2**64
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
{code}
If PR's are welcome i'm more than happy to provide one. :)
Best regards,
Eric
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
