[GLUTEN-8471][VL] Fix usage of uninitialized variables #8470

jkhaliqi · 2025-01-08T19:48:04Z

Use of Uninitialized Variables

false positives(mainly since the file was most likely deleted and only contains 1480 lines now. Went over that file and tried to find any other Uninitialized Variables and change them accordingly. There is 9 FP below and also 9 changes in that file after taking a look at what could have been the line numbers):
cpp/velox/substrait/SubstraitToVeloxPlan.cc:1902
cpp/velox/substrait/SubstraitToVeloxPlan.cc:1762
cpp/velox/substrait/SubstraitToVeloxPlan.cc:1680
cpp/velox/substrait/SubstraitToVeloxPlan.cc:1653
cpp/velox/substrait/SubstraitToVeloxPlan.cc:1927
pp/velox/substrait/SubstraitToVeloxPlan.cc:2539
cpp/velox/substrait/SubstraitToVeloxPlan.cc:1960
cpp/velox/substrait/SubstraitToVeloxPlan.cc:1932
cpp/velox/substrait/SubstraitToVeloxPlan.cc:2441

(Fixes: #8471)

github-actions · 2025-01-08T19:48:21Z

Thanks for opening a pull request!

Could you open an issue for this pull request on Github Issues?

https://github.com/apache/incubator-gluten/issues

Then could you also rename commit message and pull request title in the following format?

[GLUTEN-${ISSUES_ID}][COMPONENT]feat/fix: ${detailed message}

See also:

Other pull requests

github-actions · 2025-01-08T20:02:39Z

#8471

FelixYBW · 2025-01-08T20:06:13Z

cpp/velox/compute/WholeStageResultIterator.cc

  while (true) {
    auto future = velox::ContinueFuture::makeEmpty();
+    if (task == nullptr) {


task_?

we shouldn't check null in side of the loop.

majetideepak

@jkhaliqi Let's limit changes to Use of Uninitialized Variables
Please update the title. "[VL] Fix usage of uninitialized variables"

majetideepak · 2025-01-08T20:00:17Z

cpp/velox/compute/WholeStageResultIterator.cc

  while (true) {
    auto future = velox::ContinueFuture::makeEmpty();
+    if (task == nullptr) {


Why add this check?

Use of Uninitialized Variable@cpp/velox/compute/WholeStageResultIterator.cc:212
Use of Uninitialized Variable@cpp/velox/compute/WholeStageResultIterator.cc:209
Assuming the task_ needed to be checked to make sure it was not nullptr in order to be used for task_->next(&future); and task_->taskId()

majetideepak · 2025-01-08T20:01:35Z

cpp/velox/memory/VeloxMemoryManager.cc

@@ -125,8 +125,8 @@ class ListenableArbitrator : public velox::memory::MemoryArbitrator {

  uint64_t shrinkCapacity(uint64_t targetBytes, bool allowSpill, bool allowAbort) override {
    velox::memory::ScopedMemoryArbitrationContext ctx{};
-    facebook::velox::exec::MemoryReclaimer::Stats status;
-    velox::memory::MemoryPool* pool;
+    facebook::velox::exec::MemoryReclaimer::Stats status{};


This change is not required. facebook::velox::exec::MemoryReclaimer::Stats initializes its fields.

majetideepak · 2025-01-08T20:01:44Z

cpp/velox/memory/VeloxMemoryManager.cc

@@ -141,7 +141,7 @@ class ListenableArbitrator : public velox::memory::MemoryArbitrator {
  }

  Stats stats() const override {
-    Stats stats; // no-op
+    Stats stats{}; // no-op


not required.

majetideepak · 2025-01-08T20:02:18Z

cpp/velox/memory/VeloxMemoryManager.cc

@@ -215,7 +215,11 @@ class ArbitratorFactoryRegister {
 };

 VeloxMemoryManager::VeloxMemoryManager(const std::string& kind, std::unique_ptr<AllocationListener> listener)
-    : MemoryManager(kind), listener_(std::move(listener)) {
+    : MemoryManager(kind) {
+  if (listener == nullptr) {


Why add this check? Is it related to the CVE?

Yeah there was some CVE's for
VeloxMemoryManager.cc:243
Use of Uninitialized Variable@cpp/velox/memory/VeloxMemoryManager.cc:250
Use of Uninitialized Variable@cpp/velox/memory/VeloxMemoryManager.cc:253
which was around here so I figured it might be with the listener being passed in as nullptr so figured I would check that before the method goes in.

majetideepak · 2025-01-08T20:02:34Z

cpp/velox/memory/VeloxMemoryManager.cc

@@ -253,7 +257,10 @@ VeloxMemoryManager::VeloxMemoryManager(const std::string& kind, std::unique_ptr<

 namespace {
 MemoryUsageStats collectVeloxMemoryUsageStats(const velox::memory::MemoryPool* pool) {
-  MemoryUsageStats stats;
+  if (pool == nullptr) {


No related to the CVE.

Use of Uninitialized Variable@cpp/velox/memory/VeloxMemoryManager.cc:255
Use of Uninitialized Variable@cpp/velox/memory/VeloxMemoryManager.cc:256
Use of Uninitialized Variable@cpp/velox/memory/VeloxMemoryManager.cc:257
55 being the pool in the paramter I figured I would check if that is nullPtr
then stats being 56 I just added {}
and then 57 was using pool->usedBytes so the assuming the nullptr check above should fix that

removed as false positive.

majetideepak · 2025-01-08T20:08:32Z

cpp/velox/tests/Substrait2VeloxPlanValidatorTest.cc

@@ -41,7 +41,7 @@ class Substrait2VeloxPlanValidatorTest : public exec::test::HiveConnectorTestBas
  bool validatePlan(std::string file) {
    std::string subPlanPath = FilePathGenerator::getDataFilePath(file);

-    ::substrait::Plan substraitPlan;
+    ::substrait::Plan substraitPlan{};


not required.

Use of Uninitialized Variable@cpp/velox/tests/Substrait2VeloxPlanValidatorTest.cc:45

majetideepak · 2025-01-08T20:08:39Z

cpp/velox/substrait/SubstraitToVeloxPlan.cc

  if (!readRel.has_filter()) {
    tableHandle = std::make_shared<connector::hive::HiveTableHandle>(
        kHiveConnectorId, "hive_table", filterPushdownEnabled, connector::hive::SubfieldFilters{}, nullptr);
  } else {
-    connector::hive::SubfieldFilters subfieldFilters;
+    connector::hive::SubfieldFilters subfieldFilters{};


not required.

majetideepak · 2025-01-08T20:08:52Z

cpp/velox/substrait/SubstraitToVeloxPlan.cc

@@ -866,7 +866,7 @@ const core::WindowNode::Frame SubstraitToVeloxPlanConverter::createWindowFrame(
    const ::substrait::Expression_WindowFunction_Bound& upper_bound,
    const ::substrait::WindowType& type,
    const RowTypePtr& inputType) {
-  core::WindowNode::Frame frame;
+  core::WindowNode::Frame frame{};


not required.

majetideepak · 2025-01-08T20:09:18Z

cpp/velox/substrait/SubstraitToVeloxExpr.cc

@@ -312,7 +312,7 @@ std::shared_ptr<const core::ConstantTypedExpr> SubstraitVeloxExprConverter::lite
  std::vector<variant> variants;
  variants.reserve(literals.size());
  VELOX_CHECK_GE(literals.size(), 0, "List should have at least one item.");
-  std::optional<TypePtr> literalType;
+  std::optional<TypePtr> literalType = std::nullopt;


not required. std::optional is initialized by default.

Use of Uninitialized Variable@cpp/velox/substrait/SubstraitToVeloxExpr.cc:318
if (!literalType.has_value()) {
figured i'd explicitly initialize as well

Not required. The constructor of std::optional does this.

majetideepak · 2025-01-08T20:10:04Z

cpp/velox/substrait/SubstraitToVeloxExpr.cc

@@ -433,7 +433,7 @@ VectorPtr SubstraitVeloxExprConverter::literalsToVector(
    // Handle EmptyList and List together since the children could be either case.
    case ::substrait::Expression_Literal::LiteralTypeCase::kEmptyList:
    case ::substrait::Expression_Literal::LiteralTypeCase::kList: {
-      ArrayVectorPtr elements;
+      ArrayVectorPtr elements = nullptr;


not required. ArrayVectorPtr is a std::shared_ptr which is initialized by default.
Same below for RowVectorPtr and MapVectorPtr

Use of Uninitialized Variable@cpp/velox/substrait/SubstraitToVeloxExpr.cc:448 (if (!elements)
454 (return elements;)
472 (!mapVector)
478 (return mapVector;)
485 (!rowVector)
491 (return rowVector;)
Im assuming it would initialize it by default but the errors happening at these lines all point to these variables so I figured to explicitly call it nullptr? Should it be resolved some other way, or we can call these false positives?

false positive.

zhztheplayer · 2025-01-10T13:14:27Z

cpp/velox/compute/WholeStageResultIterator.cc

@@ -192,10 +192,13 @@ std::shared_ptr<velox::core::QueryCtx> WholeStageResultIterator::createNewVeloxQ

 std::shared_ptr<ColumnarBatch> WholeStageResultIterator::next() {
  tryAddSplitsToTask();
+  if (task_ == nullptr) {


Can we throw exception instead of returning nullptr?

majetideepak · 2025-01-10T14:43:00Z

cpp/velox/compute/WholeStageResultIterator.cc

  if (task_->isFinished()) {
    return nullptr;
  }
-  velox::RowVectorPtr vector;
+  velox::RowVectorPtr vector = nullptr;


redundant since RowVectorPtr is a shared pointer.

std::shared_ptr<connector::hive::HiveBucketProperty> bucketProperty = nullptr;
I see the above as well, but not sure if assignment should be removed. Will not add in nullptr for share pointers in this PR for Uninitialized variables though, should be updated as false positives

majetideepak · 2025-01-10T14:44:40Z

cpp/velox/memory/VeloxMemoryManager.cc

+  if (listener == nullptr) {
+    throw gluten::GlutenException("VeloxMemoryManager failed");
+  }
+  listener_(std::move(listener))


Why move this here?
You can keep it as is and check for listener_ above

majetideepak · 2025-01-10T14:45:41Z

cpp/velox/memory/VeloxMemoryManager.cc

@@ -265,20 +272,25 @@ MemoryUsageStats collectVeloxMemoryUsageStats(const velox::memory::MemoryPool* p
 }

 MemoryUsageStats collectGlutenAllocatorMemoryUsageStats(const MemoryAllocator* allocator) {
-  MemoryUsageStats stats;
+  MemoryUsageStats stats{};


This is redundant.

majetideepak · 2025-01-10T14:46:28Z

cpp/velox/substrait/SubstraitToVeloxExpr.cc

@@ -312,7 +312,7 @@ std::shared_ptr<const core::ConstantTypedExpr> SubstraitVeloxExprConverter::lite
  std::vector<variant> variants;
  variants.reserve(literals.size());
  VELOX_CHECK_GE(literals.size(), 0, "List should have at least one item.");
-  std::optional<TypePtr> literalType;
+  std::optional<TypePtr> literalType = std::nullopt;


Not required. The constructor of std::optional does this.

majetideepak · 2025-01-10T14:46:47Z

cpp/velox/substrait/SubstraitToVeloxExpr.cc

@@ -433,7 +433,7 @@ VectorPtr SubstraitVeloxExprConverter::literalsToVector(
    // Handle EmptyList and List together since the children could be either case.
    case ::substrait::Expression_Literal::LiteralTypeCase::kEmptyList:
    case ::substrait::Expression_Literal::LiteralTypeCase::kList: {
-      ArrayVectorPtr elements;
+      ArrayVectorPtr elements = nullptr;


false positive.

Use of Uninitialized Variables

github-actions bot added the VELOX label Jan 8, 2025

FelixYBW changed the title ~~Fix C++ CVEs~~ [VL] Fix C++ CVEs Jan 8, 2025

FelixYBW changed the title ~~[VL] Fix C++ CVEs~~ [GLUTEN-8471][VL] Fix C++ CVEs Jan 8, 2025

FelixYBW reviewed Jan 8, 2025

View reviewed changes

majetideepak reviewed Jan 8, 2025

View reviewed changes

jkhaliqi force-pushed the jk_cve_variables branch 2 times, most recently from c3b05d2 to bdab7c5 Compare January 8, 2025 22:29

jkhaliqi changed the title ~~[GLUTEN-8471][VL] Fix C++ CVEs~~ [GLUTEN-8471][VL] Fix usage of uninitialized variables Jan 8, 2025

jkhaliqi force-pushed the jk_cve_variables branch from bdab7c5 to 53ccac0 Compare January 8, 2025 22:30

zhztheplayer reviewed Jan 10, 2025

View reviewed changes

majetideepak reviewed Jan 10, 2025

View reviewed changes

jkhaliqi force-pushed the jk_cve_variables branch 3 times, most recently from 4750239 to 41f2e96 Compare January 10, 2025 21:13

[VL] Fix usage of uninitialized variables

8db3e30

Use of Uninitialized Variables

jkhaliqi force-pushed the jk_cve_variables branch from 41f2e96 to 8db3e30 Compare January 10, 2025 22:16

majetideepak approved these changes Jan 10, 2025

View reviewed changes

FelixYBW approved these changes Jan 11, 2025

View reviewed changes

FelixYBW merged commit a7301e1 into apache:main Jan 11, 2025
47 of 48 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GLUTEN-8471][VL] Fix usage of uninitialized variables #8470

[GLUTEN-8471][VL] Fix usage of uninitialized variables #8470

jkhaliqi commented Jan 8, 2025 •

edited

Loading

github-actions bot commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

FelixYBW Jan 8, 2025

majetideepak left a comment

majetideepak Jan 8, 2025

jkhaliqi Jan 8, 2025

majetideepak Jan 8, 2025

jkhaliqi Jan 8, 2025

majetideepak Jan 8, 2025

jkhaliqi Jan 8, 2025

majetideepak Jan 8, 2025

jkhaliqi Jan 8, 2025

majetideepak Jan 8, 2025

jkhaliqi Jan 8, 2025

jkhaliqi Jan 10, 2025

majetideepak Jan 8, 2025

jkhaliqi Jan 8, 2025 •

edited

Loading

majetideepak Jan 8, 2025

majetideepak Jan 8, 2025

majetideepak Jan 8, 2025

jkhaliqi Jan 8, 2025

majetideepak Jan 10, 2025

majetideepak Jan 8, 2025

jkhaliqi Jan 8, 2025

majetideepak Jan 10, 2025

zhztheplayer Jan 10, 2025

majetideepak Jan 10, 2025

jkhaliqi Jan 10, 2025

majetideepak Jan 10, 2025

majetideepak Jan 10, 2025

majetideepak Jan 10, 2025

majetideepak Jan 10, 2025

[GLUTEN-8471][VL] Fix usage of uninitialized variables #8470

[GLUTEN-8471][VL] Fix usage of uninitialized variables #8470

Conversation

jkhaliqi commented Jan 8, 2025 • edited Loading

github-actions bot commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

Choose a reason for hiding this comment

majetideepak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkhaliqi Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkhaliqi commented Jan 8, 2025 •

edited

Loading

jkhaliqi Jan 8, 2025 •

edited

Loading