[THRIFT-5609] TJSONProtocol is unsafe to be used with TDeserializerPool

• Type: Bug
• Status: Closed
• Priority: Major
• Resolution: Fixed
• Affects Version/s: 0.16.0
• Fix Version/s: 0.17.0
• Component/s: Go - Library
• Assignee: Yuxuan Wang
• Reporter: Yuxuan Wang
• Created: 05/Aug/22 21:42
• Updated: 26/Oct/22 23:46
• Resolved: 06/Aug/22 15:25

Description

We just realized that when using TJSONProtocol with TDeserializerPool like this:

var deserializerPool = thrift.NewTDeserializerPoolSizeFactory(1024, thrift.NewTJSONProtocolFactory())

It only works when it never fails (e.g. it never encounters invalid json blobs). Whenever a failure happens, because of the internal state stack and other states in TJSONProtocol, it will be put back into the pool with the wrong state, and when it's next used it will fail again.

There are a few approaches we can take to fix it:

1. The simplest "fix" would be just document in TDeserializerPool and TSerializerPool that TJSONProtocol and TSimpleJSONProtocol are not safe to be used, and maybe provide some examples of how to use them (only pool the TMemoryBuffer and recreate TProtocol every time)
2. In TJSONProtocol and TSimpleJSONProtocol's [Read|Write][Message|Struct]Begin, implicitly reset the internal state. But I'm not sure how safe that is
3. Make a breaking change to TProtocol to add a Reset (or maybe ResetState) API to be used by TDeserializer/TSerializer (similar to how they always reset the TMemoryBuffer before doing anything)
4. Similar to 3 but less breaking, just add a Reset/ResetState to TJSONProtocol and TSimpleJSONProtocol (but not TProtocol), and in TDeserializer/TSerializer do a type assertion and call the Reset API on the protocol if it exists.

Issue Links

• links to: GitHub Pull Request #2639