Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[C++] PollInfo crashes if given a nullptr FlightInfo #39690

Closed
lidavidm opened this issue Jan 18, 2024 · 2 comments · Fixed by #39711
Closed

[C++] PollInfo crashes if given a nullptr FlightInfo #39690

lidavidm opened this issue Jan 18, 2024 · 2 comments · Fixed by #39711
Assignees
Labels
Component: C++ Critical Fix Bugfixes for security vulnerabilities, crashes, or invalid data. Type: bug
Milestone

Comments

@lidavidm
Copy link
Member

Describe the bug, including details regarding any error messages, version, and platform.

This should be perfectly acceptable.

Component(s)

C++

@kou
Copy link
Member

kou commented Jan 18, 2024

Oh, sorry.

I found this:

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();

Do you know more missing nullptr checks?

@lidavidm
Copy link
Member Author

I think the one I ran into was during serialization or deserialization.

RETURN_NOT_OK(ToProto(*info.info, pb_info->mutable_info()));

@lidavidm lidavidm self-assigned this Jan 19, 2024
lidavidm added a commit that referenced this issue Jan 22, 2024
### 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>
@lidavidm lidavidm added this to the 16.0.0 milestone Jan 22, 2024
@pitrou pitrou modified the milestones: 16.0.0, 15.0.1 Feb 14, 2024
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
…pache#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: apache#39673.
* Closes: apache#39690

Authored-by: David Li <li.davidm96@gmail.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
raulcd pushed a commit that referenced this issue Feb 20, 2024
### 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>
@amoeba amoeba added the Critical Fix Bugfixes for security vulnerabilities, crashes, or invalid data. label Feb 28, 2024
zanmato1984 pushed a commit to zanmato1984/arrow that referenced this issue Feb 28, 2024
…pache#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: apache#39673.
* Closes: apache#39690

Authored-by: David Li <li.davidm96@gmail.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
thisisnic pushed a commit to thisisnic/arrow that referenced this issue Mar 8, 2024
…pache#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: apache#39673.
* Closes: apache#39690

Authored-by: David Li <li.davidm96@gmail.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: C++ Critical Fix Bugfixes for security vulnerabilities, crashes, or invalid data. Type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants