kpumuk commented on PR #3691: URL: https://github.com/apache/thrift/pull/3691#issuecomment-5166672003
Thanks for the review. The reset-order observation is correct: I reproduced that `read(protocol, 0)` leaves an existing object’s values intact in both the Ruby and native implementations. I thought about whether this should count as the kind of failed read covered by THRIFT-6124. I am leaning towards protecting an object once deserialization has started: fields are cleared before reading the new payload, so an EOFError or another protocol failure cannot expose stale values. A zero depth is slightly different because the call is rejected before the protocol is touched. In normal use, Deserializer always starts with the default budget. When recursive reading eventually reaches zero, the target is a newly allocated child, so there is no old state to preserve. The stale-state case therefore requires application code to call `read(protocol, 0)` directly on an already-used object. Resetting in that case would mean mutating the receiver even though the read was rejected before it began; it would also change the error for frozen objects from `DEPTH_LIMIT` to `FrozenError`. On balance, I think keeping the depth check first gives the cleaner behavior. I did update the C comment—the original wording described the call-local `remaining_depth` value but was attached to the file-level default. The `write_type` arity point is also fair. This is an intentional 0.25.0 compatibility change, and the README migration note asks exact-arity custom overrides to accept and forward the optional depth. There is deliberately no hidden protocol state from which an omitted argument could inherit a caller’s budget, so omission starts a fresh default budget while recursive custom writers must pass the current one explicitly. And thanks for confirming the conclusion on standalone `thrift_native` loading. -- 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]
