[
https://issues.apache.org/jira/browse/THRIFT-5310?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17231766#comment-17231766
]
Jens Geyer edited comment on THRIFT-5310 at 11/13/20, 7:01 PM:
---------------------------------------------------------------
{quote}
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.
{quote}
The -1 is correct, since all integer types are signed in Thrift.
Putting a 255 into an i8 field (formerly known as byte) should be prevented
IMHO, but I don't know that part of the codebase good enough to give specific
advice here.
was (Author: jensg):
{quote}
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.
{quote}
The -1 is correct, since all integer types are signed in Thrift.
Putting a 255 into an i8 field (formerly known as byte) should not be possible
IMHO.
> 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)
