You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@thrift.apache.org by "kpumuk (via GitHub)" <gi...@apache.org> on 2026/10/20 03:57:46 UTC
[PR] THRIFT-6143: Normalize Ruby integer writer argument errors [thrift]
kpumuk opened a new pull request, #4027:
URL: https://github.com/apache/thrift/pull/4027
<!-- Explain the changes in the pull request below: -->
Passing `nil` to an integer writer currently raises `RuntimeError: nil argument not allowed!` in pure Ruby and `StandardError: nil argument not allowed!` in the native implementations, while other non-integer arguments raise `TypeError`. Binary protocol metadata can also be partially written before an invalid field ID, sequence ID, or container size is rejected.
Make Binary, accelerated Binary, and Compact consistently reject non-integer values, including `nil`, with `TypeError: integer argument expected`. Validate integer metadata before emitting its header, so rejected arguments leave the transport unchanged. Preserve `RangeError` for out-of-range integers and leave valid wire bytes unchanged. These are caller argument errors, not malformed-wire `ProtocolException`s.
## Benchmarks
Compared master at `db8caa7ba6c9a3ee92026a51270e6bfa1cacff26` with this change on Ruby 4.0.7/aarch64 Linux, pinned to one CPU. Each scenario writes 250,000 small structs; the table reports elapsed seconds across five warmed process trials per revision and mode, with no YJIT option passed. The native extension was rebuilt at each revision, and the extension-free runs verified it was not loaded.
Commands, run from `lib/rb`:
```sh
taskset -c 0 ruby ../../test/rb/benchmarks/protocol_benchmark.rb --small-runs 250000 --scenarios rb-bin-write-small,c-bin-write-small,rb-cmp-write-small --json
THRIFT_BENCHMARK_SKIP_NATIVE=1 taskset -c 0 ruby ../../test/rb/benchmarks/protocol_benchmark.rb --small-runs 250000 --scenarios rb-bin-write-small,rb-cmp-write-small --json
```
| Path | Master median [range], seconds | Proposed median [range], seconds | Median change |
| --- | --- | --- | --- |
| Binary, native extension loaded | 2.202 [2.164–2.255] | 2.130 [2.059–2.368] | −3.26% |
| Accelerated Binary | 1.272 [1.201–1.286] | 1.268 [1.232–1.289] | −0.28% |
| Compact, native | 1.218 [1.183–1.468] | 1.239 [1.164–1.271] | +1.73% |
| Binary, extension-free | 3.661 [3.546–4.135] | 3.546 [3.478–3.563] | −3.16% |
| Compact, extension-free | 3.832 [3.744–4.136] | 3.867 [3.833–4.616] | +0.93% |
Lower is faster. No material slowdown was established: the small Compact increases are within the observed variation. This is one workload on one Ruby version and platform, not a guarantee for every application.
<!-- We recommend you review the checklist/tips before submitting a pull request. -->
- [x] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? [THRIFT-6143](https://issues.apache.org/jira/browse/THRIFT-6143) ([Request account here](https://selfserve.apache.org/jira-account.html), not required for trivial changes)
- [x] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
- [x] Did you squash your changes to a single commit? (not required, but preferred)
- [x] Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
- [ ] If your change does not involve any code, add the CI-skip marker to the commit message to free up build resources.
<!--
The Contributing Guide at:
https://github.com/apache/thrift/blob/master/CONTRIBUTING.md
has more details and tips for committing properly.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6143: Normalize Ruby integer writer argument errors [thrift]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #4027:
URL: https://github.com/apache/thrift/pull/4027#discussion_r4325010517
##########
lib/rb/lib/thrift/protocol/binary_protocol.rb:
##########
@@ -48,73 +48,84 @@ def initialize(trans, strict_read = true, strict_write = true, max_string_size:
end
def write_message_begin(name, type, seqid)
+ raise ::TypeError, "integer argument expected" unless seqid.is_a?(Integer)
+ raise RangeError if seqid < I32_MIN || seqid > I32_MAX
+
if strict_write
raise ::TypeError, "integer argument expected" unless type.is_a?(Integer)
raise RangeError if type < BYTE_MIN || type > BYTE_MAX
trans.write([VERSION_1 | type].pack("N"))
write_string(name)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
Review Comment:
These header paths now inline packing/writing (`trans.write([..].pack(...))`) instead of reusing the existing integer writer helpers (e.g., `write_i32`, `write_i16`, `write_i32_size`). Since you already validate `seqid`/`id`/`size` before emitting any bytes, calling the helper methods here should not reintroduce partial-header writes, and it centralizes the wire encoding in one place (reducing the risk of future divergence if integer encoding logic changes). Consider switching these direct `pack` writes back to the helper methods after the upfront validation.
##########
lib/rb/lib/thrift/protocol/binary_protocol.rb:
##########
@@ -48,73 +48,84 @@ def initialize(trans, strict_read = true, strict_write = true, max_string_size:
end
def write_message_begin(name, type, seqid)
+ raise ::TypeError, "integer argument expected" unless seqid.is_a?(Integer)
+ raise RangeError if seqid < I32_MIN || seqid > I32_MAX
+
if strict_write
raise ::TypeError, "integer argument expected" unless type.is_a?(Integer)
raise RangeError if type < BYTE_MIN || type > BYTE_MAX
trans.write([VERSION_1 | type].pack("N"))
write_string(name)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
else
write_string(name)
write_byte(type)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
Review Comment:
These header paths now inline packing/writing (`trans.write([..].pack(...))`) instead of reusing the existing integer writer helpers (e.g., `write_i32`, `write_i16`, `write_i32_size`). Since you already validate `seqid`/`id`/`size` before emitting any bytes, calling the helper methods here should not reintroduce partial-header writes, and it centralizes the wire encoding in one place (reducing the risk of future divergence if integer encoding logic changes). Consider switching these direct `pack` writes back to the helper methods after the upfront validation.
##########
lib/rb/lib/thrift/protocol/binary_protocol.rb:
##########
@@ -48,73 +48,84 @@ def initialize(trans, strict_read = true, strict_write = true, max_string_size:
end
def write_message_begin(name, type, seqid)
+ raise ::TypeError, "integer argument expected" unless seqid.is_a?(Integer)
+ raise RangeError if seqid < I32_MIN || seqid > I32_MAX
+
if strict_write
raise ::TypeError, "integer argument expected" unless type.is_a?(Integer)
raise RangeError if type < BYTE_MIN || type > BYTE_MAX
trans.write([VERSION_1 | type].pack("N"))
write_string(name)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
else
write_string(name)
write_byte(type)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
end
end
def write_struct_begin(name); nil; end
def write_field_begin(name, type, id)
+ raise ::TypeError, "integer argument expected" unless id.is_a?(Integer)
+ raise RangeError if id < I16_MIN || id > I16_MAX
+
write_byte(type)
- write_i16(id)
+ trans.write([id].pack("n"))
end
def write_field_stop
write_byte(Thrift::Types::STOP)
end
def write_map_begin(ktype, vtype, size)
+ raise ::TypeError, "integer argument expected" unless size.is_a?(Integer)
+ raise RangeError if size < 0 || size > I32_MAX
+
write_byte(ktype)
write_byte(vtype)
- write_i32_size(size)
+ trans.write([size].pack("N"))
Review Comment:
These header paths now inline packing/writing (`trans.write([..].pack(...))`) instead of reusing the existing integer writer helpers (e.g., `write_i32`, `write_i16`, `write_i32_size`). Since you already validate `seqid`/`id`/`size` before emitting any bytes, calling the helper methods here should not reintroduce partial-header writes, and it centralizes the wire encoding in one place (reducing the risk of future divergence if integer encoding logic changes). Consider switching these direct `pack` writes back to the helper methods after the upfront validation.
##########
lib/rb/lib/thrift/protocol/binary_protocol.rb:
##########
@@ -48,73 +48,84 @@ def initialize(trans, strict_read = true, strict_write = true, max_string_size:
end
def write_message_begin(name, type, seqid)
+ raise ::TypeError, "integer argument expected" unless seqid.is_a?(Integer)
+ raise RangeError if seqid < I32_MIN || seqid > I32_MAX
+
if strict_write
raise ::TypeError, "integer argument expected" unless type.is_a?(Integer)
raise RangeError if type < BYTE_MIN || type > BYTE_MAX
trans.write([VERSION_1 | type].pack("N"))
write_string(name)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
else
write_string(name)
write_byte(type)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
end
end
def write_struct_begin(name); nil; end
def write_field_begin(name, type, id)
+ raise ::TypeError, "integer argument expected" unless id.is_a?(Integer)
+ raise RangeError if id < I16_MIN || id > I16_MAX
+
write_byte(type)
- write_i16(id)
+ trans.write([id].pack("n"))
Review Comment:
These header paths now inline packing/writing (`trans.write([..].pack(...))`) instead of reusing the existing integer writer helpers (e.g., `write_i32`, `write_i16`, `write_i32_size`). Since you already validate `seqid`/`id`/`size` before emitting any bytes, calling the helper methods here should not reintroduce partial-header writes, and it centralizes the wire encoding in one place (reducing the risk of future divergence if integer encoding logic changes). Consider switching these direct `pack` writes back to the helper methods after the upfront validation.
##########
lib/rb/lib/thrift/protocol/binary_protocol.rb:
##########
@@ -48,73 +48,84 @@ def initialize(trans, strict_read = true, strict_write = true, max_string_size:
end
def write_message_begin(name, type, seqid)
+ raise ::TypeError, "integer argument expected" unless seqid.is_a?(Integer)
+ raise RangeError if seqid < I32_MIN || seqid > I32_MAX
+
if strict_write
raise ::TypeError, "integer argument expected" unless type.is_a?(Integer)
raise RangeError if type < BYTE_MIN || type > BYTE_MAX
trans.write([VERSION_1 | type].pack("N"))
write_string(name)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
else
write_string(name)
write_byte(type)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
end
end
def write_struct_begin(name); nil; end
def write_field_begin(name, type, id)
+ raise ::TypeError, "integer argument expected" unless id.is_a?(Integer)
+ raise RangeError if id < I16_MIN || id > I16_MAX
+
write_byte(type)
- write_i16(id)
+ trans.write([id].pack("n"))
end
def write_field_stop
write_byte(Thrift::Types::STOP)
end
def write_map_begin(ktype, vtype, size)
+ raise ::TypeError, "integer argument expected" unless size.is_a?(Integer)
+ raise RangeError if size < 0 || size > I32_MAX
+
write_byte(ktype)
write_byte(vtype)
- write_i32_size(size)
+ trans.write([size].pack("N"))
end
def write_list_begin(etype, size)
+ raise ::TypeError, "integer argument expected" unless size.is_a?(Integer)
+ raise RangeError if size < 0 || size > I32_MAX
+
write_byte(etype)
- write_i32_size(size)
+ trans.write([size].pack("N"))
end
def write_set_begin(etype, size)
+ raise ::TypeError, "integer argument expected" unless size.is_a?(Integer)
+ raise RangeError if size < 0 || size > I32_MAX
+
write_byte(etype)
- write_i32_size(size)
+ trans.write([size].pack("N"))
Review Comment:
These header paths now inline packing/writing (`trans.write([..].pack(...))`) instead of reusing the existing integer writer helpers (e.g., `write_i32`, `write_i16`, `write_i32_size`). Since you already validate `seqid`/`id`/`size` before emitting any bytes, calling the helper methods here should not reintroduce partial-header writes, and it centralizes the wire encoding in one place (reducing the risk of future divergence if integer encoding logic changes). Consider switching these direct `pack` writes back to the helper methods after the upfront validation.
##########
lib/rb/lib/thrift/protocol/binary_protocol.rb:
##########
@@ -48,73 +48,84 @@ def initialize(trans, strict_read = true, strict_write = true, max_string_size:
end
def write_message_begin(name, type, seqid)
+ raise ::TypeError, "integer argument expected" unless seqid.is_a?(Integer)
+ raise RangeError if seqid < I32_MIN || seqid > I32_MAX
+
if strict_write
raise ::TypeError, "integer argument expected" unless type.is_a?(Integer)
raise RangeError if type < BYTE_MIN || type > BYTE_MAX
trans.write([VERSION_1 | type].pack("N"))
write_string(name)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
else
write_string(name)
write_byte(type)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
end
end
def write_struct_begin(name); nil; end
def write_field_begin(name, type, id)
+ raise ::TypeError, "integer argument expected" unless id.is_a?(Integer)
+ raise RangeError if id < I16_MIN || id > I16_MAX
+
write_byte(type)
- write_i16(id)
+ trans.write([id].pack("n"))
end
def write_field_stop
write_byte(Thrift::Types::STOP)
end
def write_map_begin(ktype, vtype, size)
+ raise ::TypeError, "integer argument expected" unless size.is_a?(Integer)
+ raise RangeError if size < 0 || size > I32_MAX
+
write_byte(ktype)
write_byte(vtype)
- write_i32_size(size)
+ trans.write([size].pack("N"))
end
def write_list_begin(etype, size)
+ raise ::TypeError, "integer argument expected" unless size.is_a?(Integer)
+ raise RangeError if size < 0 || size > I32_MAX
+
write_byte(etype)
- write_i32_size(size)
+ trans.write([size].pack("N"))
Review Comment:
These header paths now inline packing/writing (`trans.write([..].pack(...))`) instead of reusing the existing integer writer helpers (e.g., `write_i32`, `write_i16`, `write_i32_size`). Since you already validate `seqid`/`id`/`size` before emitting any bytes, calling the helper methods here should not reintroduce partial-header writes, and it centralizes the wire encoding in one place (reducing the risk of future divergence if integer encoding logic changes). Consider switching these direct `pack` writes back to the helper methods after the upfront validation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6143: Normalize Ruby integer writer argument errors [thrift]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #4027:
URL: https://github.com/apache/thrift/pull/4027#discussion_r4321398387
##########
lib/rb/lib/thrift/protocol/binary_protocol.rb:
##########
@@ -48,73 +48,84 @@ def initialize(trans, strict_read = true, strict_write = true, max_string_size:
end
def write_message_begin(name, type, seqid)
+ raise ::TypeError, "integer argument expected" unless seqid.is_a?(Integer)
+ raise RangeError if seqid < I32_MIN || seqid > I32_MAX
+
if strict_write
raise ::TypeError, "integer argument expected" unless type.is_a?(Integer)
raise RangeError if type < BYTE_MIN || type > BYTE_MAX
trans.write([VERSION_1 | type].pack("N"))
write_string(name)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
else
write_string(name)
write_byte(type)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
end
Review Comment:
In the `strict_write == false` branch, `write_string(name)` runs before `write_byte(type)`. If `type` is invalid (e.g., non-integer/out-of-range), `write_byte` will raise after `name` has already been written, leaving a partially-mutated transport—contrary to the PR goal of leaving the transport unchanged when rejecting invalid metadata. Consider validating `type` (and range) before any writes in the non-strict branch (e.g., by precomputing a checked byte value), then writing `name`, `type`, and `seqid` once all validations have succeeded.
##########
lib/rb/lib/thrift/protocol/binary_protocol.rb:
##########
@@ -48,73 +48,84 @@ def initialize(trans, strict_read = true, strict_write = true, max_string_size:
end
def write_message_begin(name, type, seqid)
+ raise ::TypeError, "integer argument expected" unless seqid.is_a?(Integer)
+ raise RangeError if seqid < I32_MIN || seqid > I32_MAX
+
if strict_write
raise ::TypeError, "integer argument expected" unless type.is_a?(Integer)
raise RangeError if type < BYTE_MIN || type > BYTE_MAX
trans.write([VERSION_1 | type].pack("N"))
write_string(name)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
else
write_string(name)
write_byte(type)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
end
end
def write_struct_begin(name); nil; end
def write_field_begin(name, type, id)
+ raise ::TypeError, "integer argument expected" unless id.is_a?(Integer)
+ raise RangeError if id < I16_MIN || id > I16_MAX
+
write_byte(type)
- write_i16(id)
+ trans.write([id].pack("n"))
end
def write_field_stop
write_byte(Thrift::Types::STOP)
end
def write_map_begin(ktype, vtype, size)
+ raise ::TypeError, "integer argument expected" unless size.is_a?(Integer)
+ raise RangeError if size < 0 || size > I32_MAX
+
write_byte(ktype)
write_byte(vtype)
- write_i32_size(size)
+ trans.write([size].pack("N"))
Review Comment:
This method now guards `size` before writing, but it can still partially write if `ktype` is valid and `vtype` is invalid (since `ktype` is written before `vtype` is validated/written). If the intent is that rejected arguments leave the transport unchanged, consider validating both `ktype` and `vtype` (type + range) up-front (before any `write_byte` calls) and only then emitting bytes. The same pattern applies to other container-begin methods that write an element type before all header values are known-valid.
##########
lib/rb/lib/thrift/protocol/binary_protocol.rb:
##########
@@ -48,73 +48,84 @@ def initialize(trans, strict_read = true, strict_write = true, max_string_size:
end
def write_message_begin(name, type, seqid)
+ raise ::TypeError, "integer argument expected" unless seqid.is_a?(Integer)
+ raise RangeError if seqid < I32_MIN || seqid > I32_MAX
+
if strict_write
raise ::TypeError, "integer argument expected" unless type.is_a?(Integer)
raise RangeError if type < BYTE_MIN || type > BYTE_MAX
trans.write([VERSION_1 | type].pack("N"))
write_string(name)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
else
write_string(name)
write_byte(type)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
end
end
def write_struct_begin(name); nil; end
def write_field_begin(name, type, id)
+ raise ::TypeError, "integer argument expected" unless id.is_a?(Integer)
+ raise RangeError if id < I16_MIN || id > I16_MAX
+
write_byte(type)
- write_i16(id)
+ trans.write([id].pack("n"))
end
def write_field_stop
write_byte(Thrift::Types::STOP)
end
def write_map_begin(ktype, vtype, size)
+ raise ::TypeError, "integer argument expected" unless size.is_a?(Integer)
+ raise RangeError if size < 0 || size > I32_MAX
+
write_byte(ktype)
write_byte(vtype)
- write_i32_size(size)
+ trans.write([size].pack("N"))
end
def write_list_begin(etype, size)
+ raise ::TypeError, "integer argument expected" unless size.is_a?(Integer)
+ raise RangeError if size < 0 || size > I32_MAX
Review Comment:
The size-validation logic is now duplicated across `write_map_begin`, `write_list_begin`, and `write_set_begin` while a private `write_i32_size(size)` already exists (and likely similar range logic appears elsewhere). To reduce duplication and keep error semantics consistent as this evolves, consider extracting a single helper (e.g., `validate_i32_size(size)` that returns `size`) and using it in these methods (and inside `write_i32_size`) so there is one authoritative place for the check/message.
##########
lib/rb/lib/thrift/protocol/binary_protocol.rb:
##########
@@ -48,73 +48,84 @@ def initialize(trans, strict_read = true, strict_write = true, max_string_size:
end
def write_message_begin(name, type, seqid)
+ raise ::TypeError, "integer argument expected" unless seqid.is_a?(Integer)
+ raise RangeError if seqid < I32_MIN || seqid > I32_MAX
+
if strict_write
raise ::TypeError, "integer argument expected" unless type.is_a?(Integer)
raise RangeError if type < BYTE_MIN || type > BYTE_MAX
trans.write([VERSION_1 | type].pack("N"))
write_string(name)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
else
write_string(name)
write_byte(type)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
end
end
def write_struct_begin(name); nil; end
def write_field_begin(name, type, id)
+ raise ::TypeError, "integer argument expected" unless id.is_a?(Integer)
+ raise RangeError if id < I16_MIN || id > I16_MAX
+
write_byte(type)
- write_i16(id)
+ trans.write([id].pack("n"))
end
def write_field_stop
write_byte(Thrift::Types::STOP)
end
def write_map_begin(ktype, vtype, size)
+ raise ::TypeError, "integer argument expected" unless size.is_a?(Integer)
+ raise RangeError if size < 0 || size > I32_MAX
+
write_byte(ktype)
write_byte(vtype)
- write_i32_size(size)
+ trans.write([size].pack("N"))
end
def write_list_begin(etype, size)
+ raise ::TypeError, "integer argument expected" unless size.is_a?(Integer)
+ raise RangeError if size < 0 || size > I32_MAX
+
write_byte(etype)
- write_i32_size(size)
+ trans.write([size].pack("N"))
end
def write_set_begin(etype, size)
+ raise ::TypeError, "integer argument expected" unless size.is_a?(Integer)
+ raise RangeError if size < 0 || size > I32_MAX
Review Comment:
The size-validation logic is now duplicated across `write_map_begin`, `write_list_begin`, and `write_set_begin` while a private `write_i32_size(size)` already exists (and likely similar range logic appears elsewhere). To reduce duplication and keep error semantics consistent as this evolves, consider extracting a single helper (e.g., `validate_i32_size(size)` that returns `size`) and using it in these methods (and inside `write_i32_size`) so there is one authoritative place for the check/message.
##########
lib/rb/lib/thrift/protocol/binary_protocol.rb:
##########
@@ -48,73 +48,84 @@ def initialize(trans, strict_read = true, strict_write = true, max_string_size:
end
def write_message_begin(name, type, seqid)
+ raise ::TypeError, "integer argument expected" unless seqid.is_a?(Integer)
+ raise RangeError if seqid < I32_MIN || seqid > I32_MAX
+
if strict_write
raise ::TypeError, "integer argument expected" unless type.is_a?(Integer)
raise RangeError if type < BYTE_MIN || type > BYTE_MAX
trans.write([VERSION_1 | type].pack("N"))
write_string(name)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
else
write_string(name)
write_byte(type)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
end
end
def write_struct_begin(name); nil; end
def write_field_begin(name, type, id)
+ raise ::TypeError, "integer argument expected" unless id.is_a?(Integer)
+ raise RangeError if id < I16_MIN || id > I16_MAX
+
write_byte(type)
- write_i16(id)
+ trans.write([id].pack("n"))
end
def write_field_stop
write_byte(Thrift::Types::STOP)
end
def write_map_begin(ktype, vtype, size)
+ raise ::TypeError, "integer argument expected" unless size.is_a?(Integer)
+ raise RangeError if size < 0 || size > I32_MAX
Review Comment:
The size-validation logic is now duplicated across `write_map_begin`, `write_list_begin`, and `write_set_begin` while a private `write_i32_size(size)` already exists (and likely similar range logic appears elsewhere). To reduce duplication and keep error semantics consistent as this evolves, consider extracting a single helper (e.g., `validate_i32_size(size)` that returns `size`) and using it in these methods (and inside `write_i32_size`) so there is one authoritative place for the check/message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6143: Normalize Ruby integer writer argument errors [thrift]
Posted by "kpumuk (via GitHub)" <gi...@apache.org>.
kpumuk commented on code in PR #4027:
URL: https://github.com/apache/thrift/pull/4027#discussion_r4324847264
##########
lib/rb/lib/thrift/protocol/binary_protocol.rb:
##########
@@ -48,73 +48,84 @@ def initialize(trans, strict_read = true, strict_write = true, max_string_size:
end
def write_message_begin(name, type, seqid)
+ raise ::TypeError, "integer argument expected" unless seqid.is_a?(Integer)
+ raise RangeError if seqid < I32_MIN || seqid > I32_MAX
+
if strict_write
raise ::TypeError, "integer argument expected" unless type.is_a?(Integer)
raise RangeError if type < BYTE_MIN || type > BYTE_MAX
trans.write([VERSION_1 | type].pack("N"))
write_string(name)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
else
write_string(name)
write_byte(type)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
end
end
def write_struct_begin(name); nil; end
def write_field_begin(name, type, id)
+ raise ::TypeError, "integer argument expected" unless id.is_a?(Integer)
+ raise RangeError if id < I16_MIN || id > I16_MAX
+
write_byte(type)
- write_i16(id)
+ trans.write([id].pack("n"))
end
def write_field_stop
write_byte(Thrift::Types::STOP)
end
def write_map_begin(ktype, vtype, size)
+ raise ::TypeError, "integer argument expected" unless size.is_a?(Integer)
+ raise RangeError if size < 0 || size > I32_MAX
+
write_byte(ktype)
write_byte(vtype)
- write_i32_size(size)
+ trans.write([size].pack("N"))
Review Comment:
The key/value type tags come from generated schema metadata, so we treat their validity as an internal invariant rather than adding eager validation here. Passing an invalid vtype directly leaves one byte written both on master and with this PR; this is not a newly introduced behavior. Container sizes are different: this change rejects an invalid size before either tag is written. The description now states that narrower guarantee explicitly.
##########
lib/rb/lib/thrift/protocol/binary_protocol.rb:
##########
@@ -48,73 +48,84 @@ def initialize(trans, strict_read = true, strict_write = true, max_string_size:
end
def write_message_begin(name, type, seqid)
+ raise ::TypeError, "integer argument expected" unless seqid.is_a?(Integer)
+ raise RangeError if seqid < I32_MIN || seqid > I32_MAX
+
if strict_write
raise ::TypeError, "integer argument expected" unless type.is_a?(Integer)
raise RangeError if type < BYTE_MIN || type > BYTE_MAX
trans.write([VERSION_1 | type].pack("N"))
write_string(name)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
else
write_string(name)
write_byte(type)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
end
end
def write_struct_begin(name); nil; end
def write_field_begin(name, type, id)
+ raise ::TypeError, "integer argument expected" unless id.is_a?(Integer)
+ raise RangeError if id < I16_MIN || id > I16_MAX
+
write_byte(type)
- write_i16(id)
+ trans.write([id].pack("n"))
end
def write_field_stop
write_byte(Thrift::Types::STOP)
end
def write_map_begin(ktype, vtype, size)
+ raise ::TypeError, "integer argument expected" unless size.is_a?(Integer)
+ raise RangeError if size < 0 || size > I32_MAX
+
write_byte(ktype)
write_byte(vtype)
- write_i32_size(size)
+ trans.write([size].pack("N"))
end
def write_list_begin(etype, size)
+ raise ::TypeError, "integer argument expected" unless size.is_a?(Integer)
+ raise RangeError if size < 0 || size > I32_MAX
+
write_byte(etype)
- write_i32_size(size)
+ trans.write([size].pack("N"))
end
def write_set_begin(etype, size)
+ raise ::TypeError, "integer argument expected" unless size.is_a?(Integer)
+ raise RangeError if size < 0 || size > I32_MAX
Review Comment:
As with the map/list methods, these two inline guards deliberately validate the size before any header bytes are written, without an additional validation-helper dispatch. We are keeping this small duplication and using the shared specs to check consistent behavior.
##########
lib/rb/lib/thrift/protocol/binary_protocol.rb:
##########
@@ -48,73 +48,84 @@ def initialize(trans, strict_read = true, strict_write = true, max_string_size:
end
def write_message_begin(name, type, seqid)
+ raise ::TypeError, "integer argument expected" unless seqid.is_a?(Integer)
+ raise RangeError if seqid < I32_MIN || seqid > I32_MAX
+
if strict_write
raise ::TypeError, "integer argument expected" unless type.is_a?(Integer)
raise RangeError if type < BYTE_MIN || type > BYTE_MAX
trans.write([VERSION_1 | type].pack("N"))
write_string(name)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
else
write_string(name)
write_byte(type)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
end
end
def write_struct_begin(name); nil; end
def write_field_begin(name, type, id)
+ raise ::TypeError, "integer argument expected" unless id.is_a?(Integer)
+ raise RangeError if id < I16_MIN || id > I16_MAX
+
write_byte(type)
- write_i16(id)
+ trans.write([id].pack("n"))
end
def write_field_stop
write_byte(Thrift::Types::STOP)
end
def write_map_begin(ktype, vtype, size)
+ raise ::TypeError, "integer argument expected" unless size.is_a?(Integer)
+ raise RangeError if size < 0 || size > I32_MAX
Review Comment:
The two size guards are intentionally inline on this serialization path. write_i32_size also emits bytes, so it cannot provide prevalidation without writing the size ahead of the type tags; using it afterward would validate too late. A separate validation helper would add another Ruby method dispatch for these two checks. We prefer keeping the small checks inline, with regression coverage for consistent exception semantics.
##########
lib/rb/lib/thrift/protocol/binary_protocol.rb:
##########
@@ -48,73 +48,84 @@ def initialize(trans, strict_read = true, strict_write = true, max_string_size:
end
def write_message_begin(name, type, seqid)
+ raise ::TypeError, "integer argument expected" unless seqid.is_a?(Integer)
+ raise RangeError if seqid < I32_MIN || seqid > I32_MAX
+
if strict_write
raise ::TypeError, "integer argument expected" unless type.is_a?(Integer)
raise RangeError if type < BYTE_MIN || type > BYTE_MAX
trans.write([VERSION_1 | type].pack("N"))
write_string(name)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
else
write_string(name)
write_byte(type)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
end
Review Comment:
This behavior predates the PR: passing an invalid message type in non-strict mode leaves the same partial header on master. In normal client/processor paths, message types are library constants (CALL, ONEWAY, REPLY, EXCEPTION), not application values. This change addresses integer value errors and early validation of field IDs, sequence IDs, and container sizes; it does not add transactional validation of internal type tags. The description has been narrowed to make that scope explicit.
##########
lib/rb/lib/thrift/protocol/binary_protocol.rb:
##########
@@ -48,73 +48,84 @@ def initialize(trans, strict_read = true, strict_write = true, max_string_size:
end
def write_message_begin(name, type, seqid)
+ raise ::TypeError, "integer argument expected" unless seqid.is_a?(Integer)
+ raise RangeError if seqid < I32_MIN || seqid > I32_MAX
+
if strict_write
raise ::TypeError, "integer argument expected" unless type.is_a?(Integer)
raise RangeError if type < BYTE_MIN || type > BYTE_MAX
trans.write([VERSION_1 | type].pack("N"))
write_string(name)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
else
write_string(name)
write_byte(type)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
end
end
def write_struct_begin(name); nil; end
def write_field_begin(name, type, id)
+ raise ::TypeError, "integer argument expected" unless id.is_a?(Integer)
+ raise RangeError if id < I16_MIN || id > I16_MAX
+
write_byte(type)
- write_i16(id)
+ trans.write([id].pack("n"))
end
def write_field_stop
write_byte(Thrift::Types::STOP)
end
def write_map_begin(ktype, vtype, size)
+ raise ::TypeError, "integer argument expected" unless size.is_a?(Integer)
+ raise RangeError if size < 0 || size > I32_MAX
+
write_byte(ktype)
write_byte(vtype)
- write_i32_size(size)
+ trans.write([size].pack("N"))
end
def write_list_begin(etype, size)
+ raise ::TypeError, "integer argument expected" unless size.is_a?(Integer)
+ raise RangeError if size < 0 || size > I32_MAX
Review Comment:
Keeping the two checks inline here is intentional, for the same reason as in write_map_begin: the size must be validated before the element type is emitted, and a separate validation helper adds another method dispatch on this serialization path. The shared specs cover the required exception and no-partial-header behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6143: Normalize Ruby integer writer argument errors [thrift]
Posted by "kpumuk (via GitHub)" <gi...@apache.org>.
kpumuk commented on code in PR #4027:
URL: https://github.com/apache/thrift/pull/4027#discussion_r4327406056
##########
lib/rb/lib/thrift/protocol/binary_protocol.rb:
##########
@@ -48,73 +48,84 @@ def initialize(trans, strict_read = true, strict_write = true, max_string_size:
end
def write_message_begin(name, type, seqid)
+ raise ::TypeError, "integer argument expected" unless seqid.is_a?(Integer)
+ raise RangeError if seqid < I32_MIN || seqid > I32_MAX
+
if strict_write
raise ::TypeError, "integer argument expected" unless type.is_a?(Integer)
raise RangeError if type < BYTE_MIN || type > BYTE_MAX
trans.write([VERSION_1 | type].pack("N"))
write_string(name)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
else
write_string(name)
write_byte(type)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
end
end
def write_struct_begin(name); nil; end
def write_field_begin(name, type, id)
+ raise ::TypeError, "integer argument expected" unless id.is_a?(Integer)
+ raise RangeError if id < I16_MIN || id > I16_MAX
+
write_byte(type)
- write_i16(id)
+ trans.write([id].pack("n"))
end
def write_field_stop
write_byte(Thrift::Types::STOP)
end
def write_map_begin(ktype, vtype, size)
+ raise ::TypeError, "integer argument expected" unless size.is_a?(Integer)
+ raise RangeError if size < 0 || size > I32_MAX
+
write_byte(ktype)
write_byte(vtype)
- write_i32_size(size)
+ trans.write([size].pack("N"))
end
def write_list_begin(etype, size)
+ raise ::TypeError, "integer argument expected" unless size.is_a?(Integer)
+ raise RangeError if size < 0 || size > I32_MAX
+
write_byte(etype)
- write_i32_size(size)
+ trans.write([size].pack("N"))
end
def write_set_begin(etype, size)
+ raise ::TypeError, "integer argument expected" unless size.is_a?(Integer)
+ raise RangeError if size < 0 || size > I32_MAX
+
write_byte(etype)
- write_i32_size(size)
+ trans.write([size].pack("N"))
Review Comment:
The useful work is already done above: reject an invalid size before writing the element tag. Routing the accepted size through write_i32_size afterward adds another method call and repeats validation. Keeping the set header uncomplicated rather than giving those checks an encore.
##########
lib/rb/lib/thrift/protocol/binary_protocol.rb:
##########
@@ -48,73 +48,84 @@ def initialize(trans, strict_read = true, strict_write = true, max_string_size:
end
def write_message_begin(name, type, seqid)
+ raise ::TypeError, "integer argument expected" unless seqid.is_a?(Integer)
+ raise RangeError if seqid < I32_MIN || seqid > I32_MAX
+
if strict_write
raise ::TypeError, "integer argument expected" unless type.is_a?(Integer)
raise RangeError if type < BYTE_MIN || type > BYTE_MAX
trans.write([VERSION_1 | type].pack("N"))
write_string(name)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
Review Comment:
Correct: this would preserve the upfront validation. It would also send an already-checked seqid through the same type/range checks again, plus a Ruby method dispatch per message. The direct pack emits exactly the same four bytes. Once through validation is plenty.
##########
lib/rb/lib/thrift/protocol/binary_protocol.rb:
##########
@@ -48,73 +48,84 @@ def initialize(trans, strict_read = true, strict_write = true, max_string_size:
end
def write_message_begin(name, type, seqid)
+ raise ::TypeError, "integer argument expected" unless seqid.is_a?(Integer)
+ raise RangeError if seqid < I32_MIN || seqid > I32_MAX
+
if strict_write
raise ::TypeError, "integer argument expected" unless type.is_a?(Integer)
raise RangeError if type < BYTE_MIN || type > BYTE_MAX
trans.write([VERSION_1 | type].pack("N"))
write_string(name)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
else
write_string(name)
write_byte(type)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
end
end
def write_struct_begin(name); nil; end
def write_field_begin(name, type, id)
+ raise ::TypeError, "integer argument expected" unless id.is_a?(Integer)
+ raise RangeError if id < I16_MIN || id > I16_MAX
+
write_byte(type)
- write_i16(id)
+ trans.write([id].pack("n"))
Review Comment:
This runs for every field header, so the extra call is less innocent than it looks. write_i16 would recheck the field ID we just validated, only to execute this same pack("n"). Two bytes don't need a middleman.
##########
lib/rb/lib/thrift/protocol/binary_protocol.rb:
##########
@@ -48,73 +48,84 @@ def initialize(trans, strict_read = true, strict_write = true, max_string_size:
end
def write_message_begin(name, type, seqid)
+ raise ::TypeError, "integer argument expected" unless seqid.is_a?(Integer)
+ raise RangeError if seqid < I32_MIN || seqid > I32_MAX
+
if strict_write
raise ::TypeError, "integer argument expected" unless type.is_a?(Integer)
raise RangeError if type < BYTE_MIN || type > BYTE_MAX
trans.write([VERSION_1 | type].pack("N"))
write_string(name)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
else
write_string(name)
write_byte(type)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
end
end
def write_struct_begin(name); nil; end
def write_field_begin(name, type, id)
+ raise ::TypeError, "integer argument expected" unless id.is_a?(Integer)
+ raise RangeError if id < I16_MIN || id > I16_MAX
+
write_byte(type)
- write_i16(id)
+ trans.write([id].pack("n"))
end
def write_field_stop
write_byte(Thrift::Types::STOP)
end
def write_map_begin(ktype, vtype, size)
+ raise ::TypeError, "integer argument expected" unless size.is_a?(Integer)
+ raise RangeError if size < 0 || size > I32_MAX
+
write_byte(ktype)
write_byte(vtype)
- write_i32_size(size)
+ trans.write([size].pack("N"))
end
def write_list_begin(etype, size)
+ raise ::TypeError, "integer argument expected" unless size.is_a?(Integer)
+ raise RangeError if size < 0 || size > I32_MAX
+
write_byte(etype)
- write_i32_size(size)
+ trans.write([size].pack("N"))
Review Comment:
Reuse would be correct, but redundant: the helper repeats the integer and bounds checks on every list header. The remaining encoding is a single standard pack("N"), not a second implementation of an algorithm. DRY doesn't need a second lap.
##########
lib/rb/lib/thrift/protocol/binary_protocol.rb:
##########
@@ -48,73 +48,84 @@ def initialize(trans, strict_read = true, strict_write = true, max_string_size:
end
def write_message_begin(name, type, seqid)
+ raise ::TypeError, "integer argument expected" unless seqid.is_a?(Integer)
+ raise RangeError if seqid < I32_MIN || seqid > I32_MAX
+
if strict_write
raise ::TypeError, "integer argument expected" unless type.is_a?(Integer)
raise RangeError if type < BYTE_MIN || type > BYTE_MAX
trans.write([VERSION_1 | type].pack("N"))
write_string(name)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
else
write_string(name)
write_byte(type)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
end
end
def write_struct_begin(name); nil; end
def write_field_begin(name, type, id)
+ raise ::TypeError, "integer argument expected" unless id.is_a?(Integer)
+ raise RangeError if id < I16_MIN || id > I16_MAX
+
write_byte(type)
- write_i16(id)
+ trans.write([id].pack("n"))
end
def write_field_stop
write_byte(Thrift::Types::STOP)
end
def write_map_begin(ktype, vtype, size)
+ raise ::TypeError, "integer argument expected" unless size.is_a?(Integer)
+ raise RangeError if size < 0 || size > I32_MAX
+
write_byte(ktype)
write_byte(vtype)
- write_i32_size(size)
+ trans.write([size].pack("N"))
Review Comment:
write_i32_size is a validating writer, not an unchecked encoder. Calling it here buys a second inspection of the same map size and another Ruby dispatch, without changing the encoding. We already checked its papers at the entrance.
##########
lib/rb/lib/thrift/protocol/binary_protocol.rb:
##########
@@ -48,73 +48,84 @@ def initialize(trans, strict_read = true, strict_write = true, max_string_size:
end
def write_message_begin(name, type, seqid)
+ raise ::TypeError, "integer argument expected" unless seqid.is_a?(Integer)
+ raise RangeError if seqid < I32_MIN || seqid > I32_MAX
+
if strict_write
raise ::TypeError, "integer argument expected" unless type.is_a?(Integer)
raise RangeError if type < BYTE_MIN || type > BYTE_MAX
trans.write([VERSION_1 | type].pack("N"))
write_string(name)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
else
write_string(name)
write_byte(type)
- write_i32(seqid)
+ trans.write([seqid].pack("N"))
Review Comment:
The seqid hasn't become untrustworthy on its journey into the else branch. It was validated above; write_i32 would repeat those guards and add a call on the message-writing path. Keeping the direct write here too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6143: Normalize Ruby integer writer argument errors [thrift]
Posted by "kpumuk (via GitHub)" <gi...@apache.org>.
kpumuk merged PR #4027:
URL: https://github.com/apache/thrift/pull/4027
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org