This is an automated email from the ASF dual-hosted git repository. raulcd pushed a commit to branch maint-15.0.x in repository https://gitbox.apache.org/repos/asf/arrow.git
commit 87eae3d4ebab43304e3ba24bbdb386db1ec8cbc6 Author: David Li <[email protected]> AuthorDate: Mon Jan 22 09:38:57 2024 -0500 GH-39690: [C++][FlightRPC] Fix nullptr dereference in PollInfo (#39711) ### Rationale for this change The current implementation is a bit painful to use due to the lack of a move constructor. ### What changes are included in this PR? - Fix a crash in PollInfo with a nullptr FlightInfo. - Declare all necessary constructors (https://en.cppreference.com/w/cpp/language/rule_of_three) ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes, this adds new copy constructors. * Closes: #39673. * Closes: #39690 Authored-by: David Li <[email protected]> Signed-off-by: David Li <[email protected]> --- cpp/cmake_modules/FindClangTools.cmake | 3 ++- cpp/src/arrow/flight/flight_internals_test.cc | 2 ++ cpp/src/arrow/flight/serialization_internal.cc | 10 +++++++--- cpp/src/arrow/flight/types.cc | 7 ++++++- cpp/src/arrow/flight/types.h | 13 ++++++++++++- 5 files changed, 29 insertions(+), 6 deletions(-) diff --git a/cpp/cmake_modules/FindClangTools.cmake b/cpp/cmake_modules/FindClangTools.cmake index 90df60bf54..1364ccbed8 100644 --- a/cpp/cmake_modules/FindClangTools.cmake +++ b/cpp/cmake_modules/FindClangTools.cmake @@ -40,7 +40,8 @@ set(CLANG_TOOLS_SEARCH_PATHS /usr/local/bin /usr/bin "C:/Program Files/LLVM/bin" # Windows, non-conda - "$ENV{CONDA_PREFIX}/Library/bin") # Windows, conda + "$ENV{CONDA_PREFIX}/Library/bin" # Windows, conda + "$ENV{CONDA_PREFIX}/bin") # Unix, conda if(APPLE) find_program(BREW brew) if(BREW) diff --git a/cpp/src/arrow/flight/flight_internals_test.cc b/cpp/src/arrow/flight/flight_internals_test.cc index 522973bec7..a1c5250ba6 100644 --- a/cpp/src/arrow/flight/flight_internals_test.cc +++ b/cpp/src/arrow/flight/flight_internals_test.cc @@ -282,6 +282,7 @@ TEST(FlightTypes, PollInfo) { std::nullopt}, PollInfo{std::make_unique<FlightInfo>(info), FlightDescriptor::Command("poll"), 0.1, expiration_time}, + PollInfo{}, }; std::vector<std::string> reprs = { "<PollInfo info=" + info.ToString() + @@ -290,6 +291,7 @@ TEST(FlightTypes, PollInfo) { "<PollInfo info=" + info.ToString() + " descriptor=<FlightDescriptor cmd='poll'> " "progress=0.1 expiration_time=2023-06-19 03:14:06.004339000>", + "<PollInfo info=null descriptor=null progress=null expiration_time=null>", }; ASSERT_NO_FATAL_FAILURE(TestRoundtrip<pb::PollInfo>(values, reprs)); diff --git a/cpp/src/arrow/flight/serialization_internal.cc b/cpp/src/arrow/flight/serialization_internal.cc index 64a40564af..e5a7503a63 100644 --- a/cpp/src/arrow/flight/serialization_internal.cc +++ b/cpp/src/arrow/flight/serialization_internal.cc @@ -306,8 +306,10 @@ Status ToProto(const FlightInfo& info, pb::FlightInfo* pb_info) { // PollInfo Status FromProto(const pb::PollInfo& pb_info, PollInfo* info) { - ARROW_ASSIGN_OR_RAISE(auto flight_info, FromProto(pb_info.info())); - info->info = std::make_unique<FlightInfo>(std::move(flight_info)); + if (pb_info.has_info()) { + ARROW_ASSIGN_OR_RAISE(auto flight_info, FromProto(pb_info.info())); + info->info = std::make_unique<FlightInfo>(std::move(flight_info)); + } if (pb_info.has_flight_descriptor()) { FlightDescriptor descriptor; RETURN_NOT_OK(FromProto(pb_info.flight_descriptor(), &descriptor)); @@ -331,7 +333,9 @@ Status FromProto(const pb::PollInfo& pb_info, PollInfo* info) { } Status ToProto(const PollInfo& info, pb::PollInfo* pb_info) { - RETURN_NOT_OK(ToProto(*info.info, pb_info->mutable_info())); + if (info.info) { + RETURN_NOT_OK(ToProto(*info.info, pb_info->mutable_info())); + } if (info.descriptor) { RETURN_NOT_OK(ToProto(*info.descriptor, pb_info->mutable_flight_descriptor())); } diff --git a/cpp/src/arrow/flight/types.cc b/cpp/src/arrow/flight/types.cc index 9da83fa8a1..1d43c41b69 100644 --- a/cpp/src/arrow/flight/types.cc +++ b/cpp/src/arrow/flight/types.cc @@ -373,7 +373,12 @@ arrow::Result<std::unique_ptr<PollInfo>> PollInfo::Deserialize( std::string PollInfo::ToString() const { std::stringstream ss; - ss << "<PollInfo info=" << info->ToString(); + ss << "<PollInfo info="; + if (info) { + ss << info->ToString(); + } else { + ss << "null"; + } ss << " descriptor="; if (descriptor) { ss << descriptor->ToString(); diff --git a/cpp/src/arrow/flight/types.h b/cpp/src/arrow/flight/types.h index 2342c75827..790a2067dd 100644 --- a/cpp/src/arrow/flight/types.h +++ b/cpp/src/arrow/flight/types.h @@ -693,11 +693,22 @@ class ARROW_FLIGHT_EXPORT PollInfo { progress(progress), expiration_time(expiration_time) {} - explicit PollInfo(const PollInfo& other) + // Must not be explicit; to declare one we must declare all ("rule of five") + PollInfo(const PollInfo& other) // NOLINT(runtime/explicit) : info(other.info ? std::make_unique<FlightInfo>(*other.info) : NULLPTR), descriptor(other.descriptor), progress(other.progress), expiration_time(other.expiration_time) {} + PollInfo(PollInfo&& other) noexcept = default; // NOLINT(runtime/explicit) + ~PollInfo() = default; + PollInfo& operator=(const PollInfo& other) { + info = other.info ? std::make_unique<FlightInfo>(*other.info) : NULLPTR; + descriptor = other.descriptor; + progress = other.progress; + expiration_time = other.expiration_time; + return *this; + } + PollInfo& operator=(PollInfo&& other) = default; /// \brief Get the wire-format representation of this type. ///
