thrift-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jens Geyer (Jira)" <j...@apache.org>
Subject [jira] [Comment Edited] (THRIFT-5310) Ruby BinaryProtocol has invalid range checks for byte and i64
Date Mon, 16 Nov 2020 17:35:00 GMT

    [ https://issues.apache.org/jira/browse/THRIFT-5310?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17232944#comment-17232944
] 

Jens Geyer edited comment on THRIFT-5310 at 11/16/20, 5:34 PM:
---------------------------------------------------------------

> I wouldn't call it "correct". While it is correct bit-wise, a -1 is still wrong for the
receiver 
> to work with, because it does not match the intention of the sender. 255 would ofc be

> wrong as well, since it should have never gone on the wire as i8. IMHO 

Its wrong for the sender, because you cannot put 255 into a signed 8-bit integer. That's why
I wrote

> Putting a 255 into an i8 field (formerly known as byte) should be prevented IMHO

The receiver can't do much about it. It receives 8 bits and interprets the bits as specified.

> since it should have never gone on the wire as i8. IMHO 

Correct. If the field is specified as i8, some error check should prevent you from sending
it. Note that you can't change the field type/size on the fly: if it is an i8 you can't just
write an i16 because the  receiver will simply skip the data and ignore it because of the
unexpected type. 


was (Author: jensg):
> I wouldn't call it "correct". While it is correct bit-wise, a -1 is still wrong for the
receiver 
> to work with, because it does not match the intention of the sender. 255 would ofc be

> wrong as well, since it should have never gone on the wire as i8. IMHO 

Its wrong for the sender, because you cannot put 255 into a signed 8-bit integer. That's why
I wrote

> Putting a 255 into an i8 field (formerly known as byte) should be prevented IMHO

The receiver can't do much about it. It receives 8 bits and interprets the bits as specified.


> 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