[
https://issues.apache.org/jira/browse/THRIFT-5310?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17232637#comment-17232637
]
Eric-Christian Koch commented on THRIFT-5310:
---------------------------------------------
Hello [~jensg],
thank you for your feedback!
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 :)
I checked some more language libraries and it seems that all untyped languages
do suffer from this. The languages, i took a quick peek at (except ruby), had
no range checks at all. For strongly typed languages that is ofc no problem,
but for the rest?
There are some optional type validations for the generated code, but they only
check for the general type like "Is it an Integer?". Which is valid for i8 to
i64, at least in ruby.
Any advice on who i could poke? :)
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)
