[
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)
