You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "dengzhhu653 (via GitHub)" <gi...@apache.org> on 2025/04/22 00:52:00 UTC
[PR] THRIFT-5862: Validate the message size at the endpoint transport only [thrift]
dengzhhu653 opened a new pull request, #3127:
URL: https://github.com/apache/thrift/pull/3127
<!-- Explain the changes in the pull request below: -->
<!-- We recommend you review the checklist/tips before submitting a pull request. -->
- [ ] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? ([Request account here](https://selfserve.apache.org/jira-account.html), not required for trivial changes)
- [ ] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
- [ ] Did you squash your changes to a single commit? (not required, but preferred)
- [ ] 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, include `[skip ci]` anywhere in 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: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-5862: Validate the message size at the endpoint transport only [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3127:
URL: https://github.com/apache/thrift/pull/3127#issuecomment-2845752862
> for example:
Your example is broken. If someone passes in `int.Max` for len the function will try to read it w/o complaint. Exactly that should not happen. Asking `consumeReadMessageBytes()` for validation **after** you already did the `read()` it is rather pointless, because the test is intended to happen **before** we even try to read.
While I appreciate the efforts you put into this, I'm still not convinced (a) we should do this at all and (b) with the quality of the patch. If there are good arguments for a patch that let me believe in it and convince me that my doubts are unfounded, I am perfectly fine with it. But so far, you only managed to increased the latter.
--
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-5862: Validate the message size at the endpoint transport only [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3127:
URL: https://github.com/apache/thrift/pull/3127#issuecomment-2840209845
I still have problems with this one. Some layered transports do implement specific limits, the most prominent one is TFramedTransport. If we don't want to check that anymore, the whole point of the validation gets lost.
--
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-5862: Validate the message size at the endpoint transport only [thrift]
Posted by "dengzhhu653 (via GitHub)" <gi...@apache.org>.
dengzhhu653 closed pull request #3127: THRIFT-5862: Validate the message size at the endpoint transport only
URL: https://github.com/apache/thrift/pull/3127
--
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: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-5862: Validate the message size at the endpoint transport only [thrift]
Posted by "dengzhhu653 (via GitHub)" <gi...@apache.org>.
dengzhhu653 commented on PR #3127:
URL: https://github.com/apache/thrift/pull/3127#issuecomment-2820750276
> -1 Would love to hear the reason for this. Really.
Hi @Jens-G, mainly two reasons:
- In a chain of transports, every transport(current all Transports extend TEndpointTransport) updates and checks the knownMessageSize/remainingMessageSize based on the bytes fed, this is not straightforward and confused, which one should take effect, what if a transport updates the max message size via `TTransport#getConfiguration.setMaxMessageSize`.
- For a service, we should always have an endpoint transport which gets fed from external source in a chain, it's more reasonable to validate the limit here instead of all the downstream transports.
For a chain which has only TSocket, this configured limit doesn't take effect, and no clear way to mark the boundary of the message, i.e, assumption on `TTransport#flush` to set the message end.
--
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-5862: Validate the message size at the endpoint transport only [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3127:
URL: https://github.com/apache/thrift/pull/3127#issuecomment-2845706990
> For the frame limit we can get from the innerTransport's configuration if any, otherwise default, it's missing in current PR.
I was thinking about the frame size delivered with the message. That one is usually way lower than the allowed limit.
--
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-5862: Validate the message size at the endpoint transport only [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3127:
URL: https://github.com/apache/thrift/pull/3127#issuecomment-2820568957
-1 Would love to hear the reason for this. Really.
--
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-5862: Validate the message size at the endpoint transport only [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3127:
URL: https://github.com/apache/thrift/pull/3127#issuecomment-2822467530
@all Opinions?
--
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-5862: Validate the message size at the endpoint transport only [thrift]
Posted by "dengzhhu653 (via GitHub)" <gi...@apache.org>.
dengzhhu653 commented on PR #3127:
URL: https://github.com/apache/thrift/pull/3127#issuecomment-2840443918
Thank you @Jens-G! Here is my understanding, please correct me if wrong.
The `TFramedTransport` takes in `innerTransport` as the source transport, the message limit will check at the source, this `TFramedTransport` will validate the frame limit only. For the frame limit we can get from the `innerTransport` if any, otherwise default, it's missing in current PR.
> Another example would be compression implemented as a layered transport
This is a good case. TZlibTransport for example, it still checks the message limit after the de-compression. we still allow the user to implement their own message limit based on their needs, for example:
https://github.com/apache/thrift/blob/05b0659884a839e38789ba2ca5cdeb25efe6c992/lib/java/src/test/java/org/apache/thrift/protocol/ProtocolTestBase.java#L563-L567
> no matter how much more data the socket would be able to deliver. This is by design.
An issue I can think of is how to distinguish different messages a socket might send, a Hive client can re-use the same socket for future uses, either for a different request or polling the request's status from the server.
--
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-5862: Validate the message size at the endpoint transport only [thrift]
Posted by "dengzhhu653 (via GitHub)" <gi...@apache.org>.
dengzhhu653 commented on PR #3127:
URL: https://github.com/apache/thrift/pull/3127#issuecomment-2849833280
While I was trying to debug a problem, I found every transport had `knownMessageSize` and `remainingMessageSize`, in this example, TSaslServerTransport, TSocket and TMemoryInputTransport:
<img width="761" alt="Screenshot 2025-05-05 at 11 16 03" src="https://github.com/user-attachments/assets/a75ac121-ec35-4c62-9b70-dfd11f2d9e11" />
When "MaxMessageSize reached", only `remainingMessageSize` in TMemoryInputTransport was reaching the limit, the stack:
> org.apache.thrift.transport.TTransportException: MaxMessageSize reached
at org.apache.thrift.transport.TEndpointTransport.countConsumedMessageBytes(TEndpointTransport.java:96)
at org.apache.thrift.transport.TMemoryInputTransport.read(TMemoryInputTransport.java:97)
at org.apache.thrift.transport.TSaslTransport.read(TSaslTransport.java:390)
at org.apache.thrift.transport.TSaslServerTransport.read(TSaslServerTransport.java:44)
at org.apache.thrift.transport.TTransport.readAll(TTransport.java:109)
at org.apache.thrift.protocol.TBinaryProtocol.readStringBody(TBinaryProtocol.java:417)
at org.apache.thrift.protocol.TBinaryProtocol.readString(TBinaryProtocol.java:411)
at org.apache.hadoop.hive.metastore.api.StorageDescriptor$StorageDescriptorStandardScheme.read(StorageDescriptor.java:1289)
Looks it's of no use to maintain the `knownMessageSize` and `remainingMessageSize` in the TSaslServerTransport, TSocket, so I'm thinking of how to improve and trim the validation from the chain. Instead of putting the limit check on the top of the chain(in reality we don't know who will at the top), I try to put it at the bottom(the TSocket) as this PR suggests.
> the test is intended to happen before we even try to read.
yeah, this happens on the read on a string or a container as far as I know, which we have some checks on them on the protocol lawyer.
--
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