From 90c636c61a4e5ff5dd9c5d09d681bba31d33a6d4 Mon Sep 17 00:00:00 2001
From: David Li <li.davidm96@gmail.com>
Date: Mon, 22 Jan 2024 09:38:57 -0500
Subject: [PATCH] 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 <li.davidm96@gmail.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
---
 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 90df60bf541d4..1364ccbed8162 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 522973bec7231..a1c5250ba66fa 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 64a40564afd72..e5a7503a6386b 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 9da83fa8a11f2..1d43c41b69d9f 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 2342c758273a3..790a2067dd705 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.
   ///