Repository navigation
GH-55479: [C++][FlightRPC] Give gRPC real pb::FlightData messages instead of casting Arrow structs - #55480
Open
Algunenano wants to merge 2 commits into
Open
GH-55479: [C++][FlightRPC] Give gRPC real pb::FlightData messages instead of casting Arrow structs#55480Algunenano wants to merge 2 commits into
Algunenano wants to merge 2 commits into
Conversation
…es instead of casting Arrow structs WritePayload() and ReadPayload() passed a FlightPayload or internal::FlightData to gRPC as a pb::FlightData through reinterpret_cast, and SerializationTraits<pb::FlightData> cast it back. The object is not a pb::FlightData, which is undefined behavior and which Clang CFI (-fsanitize=cfi-unrelated-cast) rejects. gRPC only passes the message to SerializationTraits, so give it an empty, real pb::FlightData that sits next to a pointer to the Arrow struct, and read the struct back from it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Both helpers use non-standard-layout offsetof expressions that trigger GCC warnings promoted to errors, breaking default Debug Flight builds.
1 open issue
What changed in this PR
This PR addresses GH-55479 by passing real protobuf FlightData objects to gRPC instead of casting Arrow structs, targeting Clang control-flow integrity failures while preserving Flight’s zero-copy serialization path.
Changes:
- Wraps protobuf messages alongside pointers to Arrow payloads and receive buffers.
- Updates serialization traits to retrieve those pointers through new helper functions.
| File | Description |
|---|---|
cpp/src/arrow/flight/transport/grpc/serialization_internal.cc |
Adds message wrappers and accessors; updates read/write helpers to pass real protobuf objects. |
cpp/src/arrow/flight/transport/grpc/customize_grpc.h |
Declares the accessors and uses them in gRPC serialization traits. |
Review effort: Balanced
| // `message` points to the `message` member of one of the structs above. | ||
| const FlightPayload& FlightPayloadOfMessage(const void* message) { | ||
| return *reinterpret_cast<const FlightPayloadMessage*>( | ||
| static_cast<const char*>(message) - offsetof(FlightPayloadMessage, message)) |
|
|
offsetof() on the message wrappers, which are not standard-layout, is conditionally-supported and warns, which fails Debug builds with -Werror. GCC, Clang and MSVC support it for types without virtual base classes. The code no longer casts between unrelated types, so -Wstrict-aliasing does not need to be silenced anymore. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
5 open issues
_WIN32) rather than compiler-based. This can miss… · Newpayload/data) are left uninitialized until explicitly assigned.… · Newmessagealways points to themessagemember inside the wrapper structs.… · NewWritePayloadandReadPayload… · New
Review effort: Lite
| // offsetof() below is used on types that are not standard-layout, which is | ||
| // conditionally-supported. GCC, Clang and MSVC support it for types without | ||
| // virtual base classes, as here. | ||
| #ifndef _WIN32 |
Comment on lines
+222
to
+227
| const FlightPayload* payload; | ||
| }; | ||
|
|
||
| struct FlightDataMessage { | ||
| pb::FlightData message; | ||
| flight::internal::FlightData* data; |
Comment on lines
+234
to
+242
| return *reinterpret_cast<const FlightPayloadMessage*>( | ||
| static_cast<const char*>(message) - offsetof(FlightPayloadMessage, message)) | ||
| ->payload; | ||
| } | ||
|
|
||
| flight::internal::FlightData* FlightDataOfMessage(void* message) { | ||
| return reinterpret_cast<FlightDataMessage*>(static_cast<char*>(message) - | ||
| offsetof(FlightDataMessage, message)) | ||
| ->data; |
Comment on lines
245
to
252
| arrow::Result<bool> WritePayload( | ||
| const FlightPayload& payload, | ||
| ::grpc::ClientReaderWriter<pb::FlightData, pb::PutResult>* writer) { | ||
| RETURN_NOT_OK(payload.Validate()); | ||
| // Pretend to be pb::FlightData and intercept in SerializationTraits | ||
| return writer->Write(*reinterpret_cast<const pb::FlightData*>(&payload), | ||
| ::grpc::WriteOptions()); | ||
| FlightPayloadMessage message; | ||
| message.payload = &payload; | ||
| return writer->Write(message.message, ::grpc::WriteOptions()); | ||
| } |
Copilot stopped reviewing on behalf of
Algunenano due to an error
November 10, 2026 17:47
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
WritePayload()andReadPayload()pass aFlightPayloadorinternal::FlightDatato gRPC as apb::FlightDatathroughreinterpret_cast, andSerializationTraits<pb::FlightData>casts it back. The object is not apb::FlightData, which is undefined behavior and which Clang CFI (-fsanitize=cfi-unrelated-cast) rejects. We found this in ClickHouse, which embeds Arrow and is enabling Clang CFI in its release builds.What changes are included in this PR?
gRPC only passes the message to
SerializationTraits<pb::FlightData>, which we control. So instead of a cast,WritePayload()andReadPayload()give gRPC an empty, realpb::FlightDatathat sits next to a pointer to the Arrow struct, and the traits read the struct back throughFlightPayloadOfMessage()andFlightDataOfMessage(). No protobuf message is built or serialized, so the zero-copy path is unchanged.Are these changes tested?
By the existing
arrow-flight-test, built with-flto=thin -fwhole-program-vtables -fvisibility=hidden -fsanitize=cfi -fno-sanitize-trap=cfi -fsanitize-recover=cfi. Before: all 94 tests pass, with 9 CFI reports onarrow::flight::protocol::FlightData(the 8 casts inserialization_internal.cc, andgrpc/impl/call_op_set.hon the same object). After: all 94 tests pass and those reports are gone.Are there any user-facing changes?
No.
Was AI used for this PR?
PR code and description written by:
Reviewed before submission by: