This is an automated email from the ASF dual-hosted git repository.
lidavidm pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/main by this push:
new 3b73f438bf GH-39690: [C++][FlightRPC] Fix nullptr dereference in
PollInfo (#39711)
3b73f438bf is described below
commit 3b73f438bf0a1abc76009f017c1df9c244854aca
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.
///
