You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "AliRana30 (via GitHub)" <gi...@apache.org> on 2026/01/30 11:40:01 UTC
[PR] GH-49079: [C++] Fix null pointer dereference in SimpleTable constructor [arrow]
AliRana30 opened a new pull request, #49086:
URL: https://github.com/apache/arrow/pull/49086
## **Rationale for this change**
"table.cc" : constructor crashes if the first column is null because it attempts to call `columns[0]->length()` before any validation logic can run.
## **What changes are included in this PR?**
Added a null check for the first column in the constructor logic:
```cpp
if (columns.size() == 0 || !columns[0]) {
num_rows_ = 0;
} else {
num_rows_ = columns[0]->length();
}
## **Are these changes tested?**
Verified with existing unit tests.
## **Are there any user-facing changes?**
No.
Closes #49079
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49079: [C++] Fix null pointer dereference in SimpleTable constructor [arrow]
Posted by "AliRana30 (via GitHub)" <gi...@apache.org>.
AliRana30 commented on PR #49086:
URL: https://github.com/apache/arrow/pull/49086#issuecomment-3841531134
Post this comment on the PR:
Thank you all for the feedback! I see there are different perspectives on the best approach:
@pitrou - You mentioned "You're not supposed to pass a null pointer here." I agree that ideally callers should validate inputs. The issue was raised because there are code paths (like in some tests) where null columns could reach **SimpleTable** constructor, causing crashes. Should we close this as "won't fix" and instead add documentation that callers must validate?
@wgtmac - Your deprecation path suggestion makes sense to avoid breaking changes. Would this be the preferred approach if we do decide to add validation?
@HuaHuaY - Noted on using std::any_of for cleaner null checking. I can apply that if we proceed with validation.
Question for team: Should I:
1.Close this PR (accept null inputs are caller responsibility + add docs)
OR
2.Implement @wgtmac's deprecation path
&
3.Keep current approach with std::any_of improvement
I found it(the null pointer deference one) as an issue but I will do as you all will say ):
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49079: [C++] Fix null pointer dereference in SimpleTable constructor [arrow]
Posted by "AliRana30 (via GitHub)" <gi...@apache.org>.
AliRana30 commented on PR #49086:
URL: https://github.com/apache/arrow/pull/49086#issuecomment-3835416952
You're absolutely right @kou - changing Table::Make() to return Result<std::shared_ptr<Table>> is a backward-incompatible API change that affects all language bindings.
I see two options:
1.Keep the existing API: Maintain std::shared_ptr<Table> Table::Make(...) but replace the internal DCHECK with runtime validation that returns nullptr on invalid input (with appropriate logging).
2.Add a new method: Introduce Result<std::shared_ptr<Table>> Table::MakeSafe(...) for the new error-handling approach, while keeping the existing Make() for backward compatibility.
Which approach would you prefer? I'll also update the implementation to use ARROW_ASSIGN_OR_RAISE as suggested.
Regarding the original issue (null pointer dereference), I can address it with explicit null checks and appropriate error handling without changing the public API signature
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49079: [C++] Fix null pointer dereference in SimpleTable constructor [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #49086:
URL: https://github.com/apache/arrow/pull/49086#issuecomment-3830377042
I think that we must not accept `columns[0] == nullptr`. Caller must provide all valid chunked arrays or arrays.
If we want to check `nullptr`, we should do it in `Table::Make()` something like the following:
```diff
diff --git a/cpp/src/arrow/table.cc b/cpp/src/arrow/table.cc
index 68a8a1951f..2fd51d858e 100644
--- a/cpp/src/arrow/table.cc
+++ b/cpp/src/arrow/table.cc
@@ -260,12 +260,18 @@ std::vector<std::shared_ptr<Field>> Table::fields() const {
std::shared_ptr<Table> Table::Make(std::shared_ptr<Schema> schema,
std::vector<std::shared_ptr<ChunkedArray>> columns,
int64_t num_rows) {
+ if (std::any_of(columns.begin(), columns.end(), [](const auto& column) {return bool(column);})) {
+ return Status::Invalid(...);
+ }
return std::make_shared<SimpleTable>(std::move(schema), std::move(columns), num_rows);
}
std::shared_ptr<Table> Table::Make(std::shared_ptr<Schema> schema,
const std::vector<std::shared_ptr<Array>>& arrays,
int64_t num_rows) {
+ if (std::any_of(arrays.begin(), arrays.end(), [](const auto& array) {return bool(array);})) {
+ return Status::Invalid(...);
+ }
return std::make_shared<SimpleTable>(std::move(schema), arrays, num_rows);
}
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49079: [C++] Fix null pointer dereference in SimpleTable constructor [arrow]
Posted by "AliRana30 (via GitHub)" <gi...@apache.org>.
AliRana30 commented on PR #49086:
URL: https://github.com/apache/arrow/pull/49086#issuecomment-3841896779
@pitrou @HuaHuaY I've updated the implementation to use std::any_of as suggested by @HuaHuaY for cleaner null checking.
However, I understand @pitrou's concern that this may not be a real bug if there's no evidence in CI. The fundamental question remains: should Table::Make validate null inputs, or is that the caller's responsibility?
I'm happy to close this if the team decides validation isn't needed. Just wanted to address the code style feedback first.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49079: [C++] Fix null pointer dereference in SimpleTable constructor [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #49086:
URL: https://github.com/apache/arrow/pull/49086#issuecomment-3837868602
Let's wait for opinions from some C++ maintainers.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49079: [C++] Fix null pointer dereference in SimpleTable constructor [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #49086:
URL: https://github.com/apache/arrow/pull/49086#discussion_r2753242143
##########
cpp/src/arrow/acero/hash_aggregate_test.cc:
##########
@@ -273,7 +273,7 @@ Result<Datum> MakeGroupByOutput(const std::vector<ExecBatch>& output_batches,
sort_keys.emplace_back(static_cast<int>(i));
}
std::shared_ptr<Schema> key_schema = schema(std::move(key_fields));
- std::shared_ptr<Table> key_table = Table::Make(std::move(key_schema), key_columns);
+ std::shared_ptr<Table> key_table = Table::Make(std::move(key_schema), key_columns).ValueOrDie();
Review Comment:
Could you use `ARROW_ASSIGN_OR_RAISE()`?
```suggestion
ARROW_ASSIGN_OR_RAISE(std::shared_ptr<Table> key_table, Table::Make(std::move(key_schema), key_columns));
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49079: [C++] Fix null pointer dereference in SimpleTable constructor [arrow]
Posted by "AliRana30 (via GitHub)" <gi...@apache.org>.
AliRana30 commented on PR #49086:
URL: https://github.com/apache/arrow/pull/49086#issuecomment-3830702896
Hi @kou I have updated the PR to follow your suggestion. I've moved the validation to Table::Make using DCHECK to ensure that callers provide valid (non-null) arrays. I also reverted the change in the SimpleTable constructor to keep it consistent with the "no-nulls-accepted" policy.
Additionally, I've included a fix for the macOS CI failures caused by the missing pkg-config@0.29.2 formula in Homebrew.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49079: [C++] Fix null pointer dereference in SimpleTable constructor [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #49086:
URL: https://github.com/apache/arrow/pull/49086#issuecomment-3839484691
IMHO, these two `Table::Make` functions look more like internal functions to return a table instance from good schema and arrays. Should we add comments about its intended behavior instead of introducing breaking changes like this? We can go through a deprecation process like this:
1. In the `table.h`, define two internal functions like below and move current implementations to them to expect good schema and arrays as input.
```cpp
namespace arrow::internal {
static std::shared_ptr<Table> MakeTable(
std::shared_ptr<Schema> schema, std::vector<std::shared_ptr<ChunkedArray>> columns,
int64_t num_rows = -1);
static std::shared_ptr<Table> MakeTable(std::shared_ptr<Schema> schema,
const std::vector<std::shared_ptr<Array>>& arrays,
int64_t num_rows = -1);
} // namespace arrow::internal
```
2. Adapter current `Table::Make` functions to call the above and migrate all other references of `Table::Make` to call `arrow::internal::MakeTable`.
3. Mark current `Table::Make` as deprecated. Add new `Table::MakeTable` to return `Result<std::unique_ptr<Table>>`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49079: [C++] Fix null pointer dereference in SimpleTable constructor [arrow]
Posted by "AliRana30 (via GitHub)" <gi...@apache.org>.
AliRana30 commented on PR #49086:
URL: https://github.com/apache/arrow/pull/49086#issuecomment-3831214230
Hi @kou,
Thank you for the feedback!
**### Regarding Table::Make and DCHECK:** You are absolutely right. I have refactored Table::Make (and Table::MakeEmpty) to return Result<std::shared_ptr<Table>> instead of std::shared_ptr<Table>. I have also replaced the DCHECK statements with explicit Status::Invalid returns to ensure validation occurs in both debug and release builds. I’ve updated all call sites across the codebase to handle the new Result type, typically using ARROW_ASSIGN_OR_RAISE or .ValueOrDie() in tests.
**### Regarding the macOS CI fix**: I apologize for the oversight in mixing these changes. I have removed the pkg-config related fix from this PR and will submit it as a separate issue and PR as requested to keep this refactor focused.
Please let me know if there are any other areas that need adjustments!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49079: [C++] Fix null pointer dereference in SimpleTable constructor [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #49086:
URL: https://github.com/apache/arrow/pull/49086#issuecomment-3823267461
:warning: GitHub issue #49079 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49079: [C++] Fix null pointer dereference in SimpleTable constructor [arrow]
Posted by "AliRana30 (via GitHub)" <gi...@apache.org>.
AliRana30 commented on PR #49086:
URL: https://github.com/apache/arrow/pull/49086#issuecomment-3831279779
Hi @WillAyd,
I've added the requested tests in `cpp/src/arrow/table_test.cc` to verify the fix.
**Specifically, I added:**
- `TestTable.MakeInvalidInputs`: Verifies that `Table::Make` properly returns `Status::Invalid` (instead of crashing) when passed vectors containing `nullptr`.
- Additional assertions in the `AddColumn` and `SetColumn` tests to ensure they also reject null inputs with `Status::Invalid`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49079: [C++] Fix null pointer dereference in SimpleTable constructor [arrow]
Posted by "HuaHuaY (via GitHub)" <gi...@apache.org>.
HuaHuaY commented on code in PR #49086:
URL: https://github.com/apache/arrow/pull/49086#discussion_r2758101680
##########
cpp/src/arrow/table.cc:
##########
@@ -257,15 +261,33 @@ std::vector<std::shared_ptr<Field>> Table::fields() const {
return result;
}
-std::shared_ptr<Table> Table::Make(std::shared_ptr<Schema> schema,
- std::vector<std::shared_ptr<ChunkedArray>> columns,
- int64_t num_rows) {
+Result<std::shared_ptr<Table>> Table::Make(std::shared_ptr<Schema> schema,
+ std::vector<std::shared_ptr<ChunkedArray>> columns,
+ int64_t num_rows) {
+ if (schema->num_fields() != static_cast<int>(columns.size())) {
+ return Status::Invalid("Schema and columns have different number of fields: ",
+ schema->num_fields(), " vs ", columns.size());
+ }
+ for (const auto& column : columns) {
Review Comment:
You can use `std::ranges::any_of`(I suggest to use it but I didn't check whether the version of arrow's C++20 compiler provides this method) or `std::any_of`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49079: [C++] Fix null pointer dereference in SimpleTable constructor [arrow]
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on PR #49086:
URL: https://github.com/apache/arrow/pull/49086#issuecomment-3829334144
Can you add a test for this?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49079: [C++] Fix null pointer dereference in SimpleTable constructor [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #49086:
URL: https://github.com/apache/arrow/pull/49086#issuecomment-3840154664
> The `SimpleTable` constructor in `cpp/src/arrow/table.cc` can crash if the first column provided is `null`.
Why is that a problem? You're not supposed to pass a null pointer her.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49079: [C++] Fix null pointer dereference in SimpleTable constructor [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #49086:
URL: https://github.com/apache/arrow/pull/49086#issuecomment-3841812901
> 1.Identify which specific tests are passing null and fix those test cases?
Well, yes, but if these tests allegedly crash, why don't we see it on CI?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49079: [C++] Fix null pointer dereference in SimpleTable constructor [arrow]
Posted by "AliRana30 (via GitHub)" <gi...@apache.org>.
AliRana30 closed pull request #49086: GH-49079: [C++] Fix null pointer dereference in SimpleTable constructor
URL: https://github.com/apache/arrow/pull/49086
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49079: [C++] Fix null pointer dereference in SimpleTable constructor [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #49086:
URL: https://github.com/apache/arrow/pull/49086#issuecomment-3830990447
Why did you use `DCHECK` instead of `arrow::Status`? We can use `arrow::Status` by changing return type of `Table::Make` to `Result<std::shared_ptr<Table>>` from `std::shared_ptr<Table>`. `DCHECK` is evaluated only with debug build. Is it intentional?
> Additionally, I've included a fix for the macOS CI failures caused by the missing pkg-config@0.29.2 formula in Homebrew.
Could you work on it as a separated task (open a separated issue and PR) instead of mixing this PR? It's not related to this issue.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49079: [C++] Fix null pointer dereference in SimpleTable constructor [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #49086:
URL: https://github.com/apache/arrow/pull/49086#issuecomment-3841583288
> The issue was raised because there are code paths (like in some tests) where null columns could reach **SimpleTable** constructor, causing crashes.
Are these code paths in the Arrow C++ codebase? If so, they should be fixed.
> Should we close this as "won't fix" and instead add documentation that callers must validate?
Well, unless the docstring for an API states that a pointer is optional, it should always be non-null. This is a common convention in C and C++ codebases, I don't think it's worth mentioning it in each and every docstring.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49079: [C++] Fix null pointer dereference in SimpleTable constructor [arrow]
Posted by "AliRana30 (via GitHub)" <gi...@apache.org>.
AliRana30 commented on PR #49086:
URL: https://github.com/apache/arrow/pull/49086#issuecomment-3841753693
@pitrou, I've searched through the codebase for Table::Make call sites. Most production code looks safe - they pass columns from existing Tables or arrays built with proper error checking.
The original issue mentioned crashes "in some tests". Rather than add API validation, should I:
1.Identify which specific tests are passing null and fix those test cases?
2.Or is this a non-issue if it's only test code doing incorrect things?
If you can point me to the specific test failures or crash reports, I can create a targeted fix for those call sites.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49079: [C++] Fix null pointer dereference in SimpleTable constructor [arrow]
Posted by "AliRana30 (via GitHub)" <gi...@apache.org>.
AliRana30 commented on PR #49086:
URL: https://github.com/apache/arrow/pull/49086#issuecomment-3829092511
Hi@kou ,can you have a look at this ??
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org