Jens-G commented on code in PR #3873:
URL: https://github.com/apache/thrift/pull/3873#discussion_r4041310429
##########
lib/go/thrift/framed_transport.go:
##########
@@ -202,11 +202,14 @@ func (p *TFramedTransport) WriteString(s string) (n int,
err error) {
func (p *TFramedTransport) Flush(ctx context.Context) error {
size := p.writeBuf.Len()
- if uint64(size) > uint64(math.MaxUint32) {
- return NewTTransportException(UNKNOWN_TRANSPORT_EXCEPTION,
fmt.Sprintf("frame too large: %d bytes exceeds uint32 max", size))
- }
-
defer bufPool.put(&p.writeBuf)
+
+ // readFrame refuses a frame larger than the configured maximum, and so
does
+ // a peer holding the same configuration. The maximum is below what the
+ // 32-bit length can carry. A refused frame is dropped with the buffer.
+ if maxSize := p.cfg.GetMaxFrameSize(); int64(size) > int64(maxSize) {
Review Comment:
Done — `checkWriteFrameSize` is a package-level function taking a
`*TConfiguration` now, and all three write paths go through it: the two in
`THeaderTransport.Flush` (header frame, plain framed answer) and this one. It
stayed in `header_transport.go` next to `THeaderMaxFrameSize`; glad to move it
if you'd rather have it elsewhere.
On `MaxUint32`: I had dropped that check on purpose, because
`GetMaxFrameSize()` returns `int32`, so `int64(size) > int64(maxSize)` already
holds size below `MaxInt32` and the old check could no longer fire. What the
shared helper really adds here is `THeaderMaxFrameSize`, and that does change
behaviour, so two points for your call:
1. **The 0x3fffffff ceiling now applies to plain framed writes too.**
`readFrame` checks the configured maximum only, so with `MaxFrameSize` and
`MaxMessageSize` set above it — `math.MaxInt32`, say — a `TFramedTransport` can
now read a 1073741824-byte frame that it refuses to write. A conservative
refusal, and it takes a deliberate >1 GiB configuration to reach, but it is new.
2. **The error type changes with it.** `Flush` returned a
`TTransportException` (`UNKNOWN_TRANSPORT_EXCEPTION`), matching `readFrame` in
this file; through the helper it returns THeader's `TProtocolException` of type
`SIZE_LIMIT`. In `TSimpleServer` that costs one log line and nothing else: the
generated processor returns `ok=false` with a `ProcessorError` either way, so
`if !ok { break }` and the goroutine's `defer client.Close()` end the
connection in both cases, but only a `TTransportException` comes back out of
`processRequests` to be logged as "error processing request". #3860 already
made that trade for THeader, so I followed it; say the word if you would rather
the framed path kept its transport exception.
Tests: the over-limit case reuses `requireFlushSizeLimit` and
`flushCountingTransport` from `header_transport_test.go` instead of its own
copies. Turning `>` into `>=` in the shared check fails the framed test and
both THeader flush tests; dropping the call from `TFramedTransport.Flush` fails
only the framed one.
Rebased on master with #3860 in, so the two README notes sit next to each
other.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
