[THRIFT-4405] Incorrect handling of sequence numbers that wrap to negative

• Type: Improvement
• Status: Closed
• Priority: Major
• Resolution: Fixed
• Affects Version/s: 0.11.0
• Fix Version/s: 0.13.0
• Component/s: Test Suite

Environment

docker ubuntu-xenial

• Assignee: James E. King III
• Reporter: James E. King III
• Created: 04/Dec/17 17:02
• Updated: 16/Oct/19 22:26
• Resolved: 01/Feb/19 16:14

Description

The following tests were added:

• The cpp client for cross test did not use sequence numbers, so I added a testing protocol layer for TestClient on top of all protocols that uses a sequence ID that starts at INT32_MAX - 10, and advances until it wraps around. This caught a number of negative value handling issues.
• The cpp client verifies the sequence ID that returns matches what was sent.
• The python client was changed to use pedantic sequence ID checking on all protocols.

The following errors were identified and fixed:

• In c_glib, thrift_stored_message_protocol was limiting the sequence ID to [0..G_INTMAX]. This was changed to allow any 32-bit value, matching other implementations.
• In cpp, JSON protocol, when the server read the header, it used a uint64_t for processing; this interacted with a bugfix from 2017 (THRIFT-4138) that dropped boost::lexical_cast and switched to std::stringstream, and this corrupted the negative sequence ID.
• In python, compact protocol, a negative sequence ID was not handled properly because it is read in and written out as a "var int" which is always positive.

Documentation was added for sequence number handling.

Issue Links

• incorporates: THRIFT-4745 warning C4305: 'initializing' : truncation from '"__int64' to 'long'
• is caused by: THRIFT-4138 Fix remaining undefined behavior invalid vptr casts in C++ library
• is related to: THRIFT-4639 Sequence numbering for multiplexed protocol broken
• is related to: THRIFT-4480 NodeJS warning on binary_protocol writeMessageEnd when seqid = 0
• links to: GitHub Pull Request #1721