Algunenano commented on code in PR #55480:
URL: https://github.com/apache/arrow/pull/55480#discussion_r4537782572
##########
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:
> 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.
AFAICT, this can't be detected from the pointer. And it was already assumed
that every pb::FlightData carried an Arrow struct.
> e.g., assert that the reconstructed wrapper’s &wrapper->message == message
I'll change the wrappers to take the pointer in the constructor. Checking
`&wrapper->message == message` would always pass.
--
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]
