[THRIFT-3978] Thrift C++ runtime uses assert to prevent overflows, checks sanity only in debug builds

• Type: Bug
• Status: Open
• Priority: Major
• Resolution: Unresolved
• Affects Version/s: 0.10.0
• Component/s: C++ - Library
• Labels: security

Environment

All

• Assignee: James E. King III
• Reporter: James E. King III
• Created: 17/Nov/16 23:28
• Updated: 11/Jan/19 14:23

Description

Currently there is widespread use of assert in the thrift C++ runtime library. Some of the more disturbing cases are security related, for example checking header sizes. I recommend we eliminate assertions that are only checked in debug mode, and instead throw the appropriate exception, usually a TTransportException with CORRUPTED_DATA as the reason. If we're going to check for an overflow or a buffer overrun, we should do so in debug and release modes. Further, assertions are not easily tested whereas exceptions are.

In THRIFT-3873 apache::thrift::transport::safe_numeric_cast was added, so I also suggest changing static_cast to safe_numeric_cast where appropriate throughout the transport code to catch any overflow errors.

Another location where assert is used liberally is inside the posix Mutex implementation.

Issue Links

• relates to: THRIFT-3873 Fix various build warnings when using Visual Studio
• relates to: THRIFT-4730 Remove pthread and boost::thread library support and use std::thread for C++11
• links to: GitHub Pull Request #1228