thrift-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Eric-Christian Koch (Jira)" <j...@apache.org>
Subject [jira] [Updated] (THRIFT-5310) Ruby BinaryProtocol has invalid range checks for byte and i64
Date Fri, 13 Nov 2020 15:11:00 GMT

     [ https://issues.apache.org/jira/browse/THRIFT-5310?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]

Eric-Christian Koch updated THRIFT-5310:
----------------------------------------
    Description: 
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**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
{code}
 

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

 

Best regards,
 Eric

 

 

  was:
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

 

 


> 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
>            Priority: Major
>
> 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**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
> {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)

Mime
View raw message