Copilot commented on code in PR #55480:
URL: https://github.com/apache/arrow/pull/55480#discussion_r4528110479
##########
cpp/src/arrow/flight/transport/grpc/serialization_internal.cc:
##########
@@ -197,78 +198,111 @@ ::grpc::Status FlightDataDeserialize(ByteBuffer* buffer,
return ::grpc::Status::OK;
}
-// The pointer bitcast hack below causes legitimate warnings, silence them.
+// 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
# pragma GCC diagnostic push
-# pragma GCC diagnostic ignored "-Wstrict-aliasing"
+# pragma GCC diagnostic ignored "-Winvalid-offsetof"
#endif
-// Pointer bitcast explanation: grpc::*Writer<T>::Write() and
grpc::*Reader<T>::Read()
-// both take a T* argument (here pb::FlightData*). But they don't do anything
-// with that argument except pass it to SerializationTraits<T>::Serialize() and
-// SerializationTraits<T>::Deserialize().
-//
-// Since we control SerializationTraits<pb::FlightData>, we can interpret the
-// pointer argument whichever way we want, including cast it back to the
original type.
-// (see customize_grpc.h).
+// grpc::*Writer<T>::Write() and grpc::*Reader<T>::Read() take a T (here
+// pb::FlightData) and only pass it to SerializationTraits<T>::Serialize() and
+// SerializationTraits<T>::Deserialize(), which we control (see
customize_grpc.h).
+// So instead of building the protobuf message, we give gRPC an empty, real
+// pb::FlightData that carries the Arrow struct, and the traits read the struct
+// from it. The message must be a real pb::FlightData: an Arrow struct cast to
+// pb::FlightData is not one, which is undefined behavior and which Clang CFI
+// (`-fsanitize=cfi-unrelated-cast`) rejects.
+
+namespace {
+
+struct FlightPayloadMessage {
+ pb::FlightData message;
+ const FlightPayload* payload;
+};
+
+struct FlightDataMessage {
+ pb::FlightData message;
+ flight::internal::FlightData* data;
Review Comment:
The pointer members (`payload` / `data`) are left uninitialized until
explicitly assigned. Initializing them to `nullptr` (e.g., in-class
initializers) makes accidental misuse easier to catch and avoids UB if a code
path ever calls the accessors before assignment.
##########
cpp/src/arrow/flight/transport/grpc/serialization_internal.cc:
##########
@@ -197,78 +198,111 @@ ::grpc::Status FlightDataDeserialize(ByteBuffer* buffer,
return ::grpc::Status::OK;
}
-// The pointer bitcast hack below causes legitimate warnings, silence them.
+// 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
Review Comment:
The diagnostic guard is OS-based (`_WIN32`) rather than compiler-based. This
can miss configurations like clang-cl on Windows (where `_WIN32` is set but
Clang may still warn on invalid-offsetof). Consider guarding pragmas by
compiler (`__clang__` / `__GNUC__`) and using the appropriate pragma form so
builds don’t regress under different toolchains.
##########
cpp/src/arrow/flight/transport/grpc/serialization_internal.cc:
##########
@@ -197,78 +198,111 @@ ::grpc::Status FlightDataDeserialize(ByteBuffer* buffer,
return ::grpc::Status::OK;
}
-// The pointer bitcast hack below causes legitimate warnings, silence them.
+// 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
# pragma GCC diagnostic push
-# pragma GCC diagnostic ignored "-Wstrict-aliasing"
+# pragma GCC diagnostic ignored "-Winvalid-offsetof"
#endif
-// Pointer bitcast explanation: grpc::*Writer<T>::Write() and
grpc::*Reader<T>::Read()
-// both take a T* argument (here pb::FlightData*). But they don't do anything
-// with that argument except pass it to SerializationTraits<T>::Serialize() and
-// SerializationTraits<T>::Deserialize().
-//
-// Since we control SerializationTraits<pb::FlightData>, we can interpret the
-// pointer argument whichever way we want, including cast it back to the
original type.
-// (see customize_grpc.h).
+// grpc::*Writer<T>::Write() and grpc::*Reader<T>::Read() take a T (here
+// pb::FlightData) and only pass it to SerializationTraits<T>::Serialize() and
+// SerializationTraits<T>::Deserialize(), which we control (see
customize_grpc.h).
+// So instead of building the protobuf message, we give gRPC an empty, real
+// pb::FlightData that carries the Arrow struct, and the traits read the struct
+// from it. The message must be a real pb::FlightData: an Arrow struct cast to
+// pb::FlightData is not one, which is undefined behavior and which Clang CFI
+// (`-fsanitize=cfi-unrelated-cast`) rejects.
+
+namespace {
+
+struct FlightPayloadMessage {
+ pb::FlightData message;
+ const FlightPayload* payload;
+};
+
+struct FlightDataMessage {
+ pb::FlightData message;
+ flight::internal::FlightData* data;
+};
+
+} // namespace
+
+// `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))
+ ->payload;
+}
+
+flight::internal::FlightData* FlightDataOfMessage(void* message) {
+ return reinterpret_cast<FlightDataMessage*>(static_cast<char*>(message) -
+ offsetof(FlightDataMessage,
message))
+ ->data;
Review Comment:
These accessors assume `message` always points to the `message` member
inside the wrapper structs. If a plain `pb::FlightData` ever reaches these
functions (even accidentally), the pointer arithmetic will compute an invalid
wrapper address and dereference an invalid `payload/data` pointer. Consider
adding debug-time validation (e.g., assert that the reconstructed wrapper’s
`&wrapper->message == message` and that `payload/data` is non-null) to fail
fast and make incorrect call sites easier to diagnose.
##########
cpp/src/arrow/flight/transport/grpc/serialization_internal.cc:
##########
@@ -197,78 +198,111 @@ ::grpc::Status FlightDataDeserialize(ByteBuffer* buffer,
return ::grpc::Status::OK;
}
-// The pointer bitcast hack below causes legitimate warnings, silence them.
+// 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
# pragma GCC diagnostic push
-# pragma GCC diagnostic ignored "-Wstrict-aliasing"
+# pragma GCC diagnostic ignored "-Winvalid-offsetof"
#endif
-// Pointer bitcast explanation: grpc::*Writer<T>::Write() and
grpc::*Reader<T>::Read()
-// both take a T* argument (here pb::FlightData*). But they don't do anything
-// with that argument except pass it to SerializationTraits<T>::Serialize() and
-// SerializationTraits<T>::Deserialize().
-//
-// Since we control SerializationTraits<pb::FlightData>, we can interpret the
-// pointer argument whichever way we want, including cast it back to the
original type.
-// (see customize_grpc.h).
+// grpc::*Writer<T>::Write() and grpc::*Reader<T>::Read() take a T (here
+// pb::FlightData) and only pass it to SerializationTraits<T>::Serialize() and
+// SerializationTraits<T>::Deserialize(), which we control (see
customize_grpc.h).
+// So instead of building the protobuf message, we give gRPC an empty, real
+// pb::FlightData that carries the Arrow struct, and the traits read the struct
+// from it. The message must be a real pb::FlightData: an Arrow struct cast to
+// pb::FlightData is not one, which is undefined behavior and which Clang CFI
+// (`-fsanitize=cfi-unrelated-cast`) rejects.
+
+namespace {
+
+struct FlightPayloadMessage {
+ pb::FlightData message;
+ const FlightPayload* payload;
+};
+
+struct FlightDataMessage {
+ pb::FlightData message;
+ flight::internal::FlightData* data;
+};
+
+} // namespace
+
+// `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))
+ ->payload;
+}
+
+flight::internal::FlightData* FlightDataOfMessage(void* message) {
+ return reinterpret_cast<FlightDataMessage*>(static_cast<char*>(message) -
+ offsetof(FlightDataMessage,
message))
+ ->data;
+}
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());
}
Review Comment:
The same wrapper setup pattern is repeated across multiple `WritePayload`
and `ReadPayload` overloads. Factoring message creation into a small helper (or
a small local function) would reduce duplication and lower the chance of future
overloads diverging (e.g., missing pointer initialization or validation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
