Nick Brekhus

This commit introduces a **new feature** to the **Thrift C++2 protocol serialization** by adding `readStringCursor` functionality. This enhancement allows for more efficient handling of string data when shuffling between buffers, enabling callers to choose between cloning or directly using a cursor to avoid unnecessary copies. Specifically, the `StructuredDynamicCursorReader` in `DynamicCursorSerializer` now exposes this cursor-based string reading, and `readStringView` has been refactored to leverage it. This provides a significant performance improvement by reducing memory allocations and improving flexibility for **Thrift** users dealing with string serialization.

This commit **refactors** the **Thrift common types** by explicitly setting the underlying type of the `BaseType` enum in `thrift/common/BaseType.h` to `int16_t`. This **memory optimization** reduces its footprint to 2 bytes, impacting the size of generated conversion table elements. Additionally, it **improves error handling** within the **Thrift C++2 type conversion utilities** by updating `toTType` and `toBaseType` functions in `thrift/lib/cpp2/type/BaseType.h` to leverage `folly::throw_exception_fmt_format`. This ensures more detailed and better-formatted error messages are provided during type conversion failures.

This commit **refactors** the **Thrift C++2 protocol serialization cursors** to standardize exception handling. It migrates several methods, including `finish` in `CursorBasedSerialization.h` and `ensureTypesCompatible` and `writeFieldBegin` in `DynamicCursorSerializer.h`, from using `folly::throw_exception` to `folly::throw_exception_fmt_format`. This change improves the **error reporting and formatting capabilities** for exceptions thrown during **Thrift C++2 serialization**, allowing for more detailed and structured error messages. The update primarily affects the internal implementation of `thrift/lib/cpp2/protocol/detail/CursorBasedSerialization.h` and `thrift/lib/cpp2/protocol/detail/DynamicCursorSerializer.h`, enhancing maintainability and diagnostic clarity without altering external behavior.

This commit introduces a **critical bug fix** to prevent a **Use-After-Free (UAF) vulnerability** within the **Thrift C++2 asynchronous client library**. Specifically, it addresses an issue in the **header channel's one-way request path** where the channel's lifetime could end prematurely during callback execution. By adding a `DestructorGuard` in the `sendRequestNoResponse` function within `thrift/lib/cpp2/async/HeaderClientChannel.cpp`, the channel's lifetime is now properly extended. This change ensures the **stability and reliability of one-way RPCs**, preventing crashes and undefined behavior caused by accessing deallocated channel objects.

This commit **fixes a bug** in **Thrift protocol deserialization** related to reading string views with dynamic cursors, specifically addressing how string lengths are handled. It **refactors** the **`CompactProtocol`** to properly manage varint-encoded string lengths by introducing a public `readStringSize` method in both **`CompactProtocolReader`** and **`BinaryProtocolReader`**. The `readStringView` function in `thrift/lib/cpp2/protocol/detail/CursorBasedSerialization.h` is updated to utilize these new, consistent `readStringSize` methods, ensuring accurate buffer length checks. This change improves the correctness and robustness of string deserialization across protocols, validated by new test cases.

This commit provides a **bug fix** to prevent potential data truncation when converting a **chained `folly::IOBuf`** to `std::string`. Previously, certain callsites incorrectly assumed `IOBuf`s were unchained, leading to data loss if the `IOBuf` was composed of multiple buffers. The fix involves updating the conversion logic in `thrift/lib/cpp2/protocol/benchmark/ThriftProtocolBenchmarkHelper.cpp`, specifically within the `deserializeProtobufString` function, to use the `to<std::string>()` method, which correctly handles chained `IOBuf`s. This ensures data integrity and accurate string representation within the **Thrift protocol benchmarking** utilities.

This commit delivers a **bug fix** for **`folly::IOBufQueue`**, resolving an issue where appending an unpackable `const IOBuf` would incorrectly prevent tail space reuse, even when `allowTailReuse` was explicitly enabled. The `append` method in `folly/io/IOBufQueue.cpp` is now corrected to properly propagate the `allowTailReuse` parameter, ensuring that memory is managed efficiently by allowing the queue to reuse tail space as intended. This prevents unnecessary memory allocations that previously occurred due to the propagation failure. To validate this fix and improve test coverage, new parameterized tests are added to `folly/io/test/IOBufQueueTest.cpp` specifically targeting the `pack` behavior during append operations.

This commit **completes enum type support** within the **Thrift C++2 Type system**. It **fixes** an issue where the `makeType` function in `thrift/lib/cpp2/type/Type.h` was not correctly generating the Thrift URI for enum types, ensuring their universal names are now properly produced. This **enhances the fidelity** of the Type system, allowing enum types to be accurately identified and handled. A new test case has been added to `thrift/lib/cpp2/type/TypeTest.cpp` to verify that `Type::create` accurately processes enum types with their universal names.

This commit **fixes a pessimizing move** within the **Thrift deserialization benchmark** by removing an unnecessary `std::move` call. Specifically, the `std::move` was incorrectly applied during a map key assignment within the `buildRandomStructB` function in `thrift/test/DeserializationBench.cpp`. This **performance optimization** ensures the benchmark accurately reflects deserialization efficiency by eliminating artificial overhead. The change is a **bug fix** that improves the correctness and reliability of the benchmark's performance measurements.

This commit **refactors** the `operator[]` (subscript operator) within the **Thrift C++2 library's FieldRef mechanism**, specifically for `field_ref`, `required_field_ref`, and `terse_field_ref`. The change introduces **perfect forwarding** for the index argument of `operator[]`, ensuring that calls are correctly propagated to the underlying container type. This **enhancement** allows `thrift::FieldRef` to seamlessly support all `operator[]` overloads provided by the wrapped container, such as `std::map` or `std::vector`, including those for rvalue and const references. New test cases validate this improved functionality, ensuring broader compatibility and correctness when accessing elements of container fields via `FieldRef`.

This commit **refactors** the `co_try_wait_for` method within the **`folly::fibers::Semaphore`** class. The implementation is updated to directly return the result of `timeoutNoDiscard`, effectively **avoiding an extra coroutine frame**. This optimization improves the efficiency of asynchronous operations and **enhances caller control over exception handling**, allowing for more precise use of mechanisms like `co_nothrow` or `co_awaitTry`. The change primarily impacts the **`folly::fibers` library**, leading to more performant and flexible synchronization primitives.

This commit **refactors** the move constructor and move assignment operator for the **`folly::coro::SharedPromise` class** to address a **deadlock vulnerability**. It resolves a **TSAN-reported lock order inversion** that occurred when using the swap algorithm, where nested locks were acquired in an unsafe order during move operations. This **bug fix** improves the **robustness and correctness** of `SharedPromise` by ensuring safe concurrent access during object movement. New test cases were added to validate `SharedPromise`'s swap, move-from, and self-move behaviors.

This commit **optimizes the `BatchSemaphore` component** within `folly/fibers` by **refactoring the `co_wait` function**. It **removes an unnecessary coroutine frame** by having `co_wait` directly return the result of `co_wait_common`. This **performance improvement** specifically **reduces overhead** when callers use `co_awaitTry`, preventing cancelled exceptions from traversing an extra, redundant frame. The change primarily affects the **internal implementation of `BatchSemaphore`**, leading to more efficient exception handling in fiber-based asynchronous operations.