[GLUTEN-8471][VL] Fix usage of uninitialized variables#8470
[GLUTEN-8471][VL] Fix usage of uninitialized variables#8470FelixYBW merged 1 commit intoapache:mainfrom
Conversation
|
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? See also: |
| velox::RowVectorPtr vector = nullptr; | ||
| while (true) { | ||
| auto future = velox::ContinueFuture::makeEmpty(); | ||
| if (task == nullptr) { |
There was a problem hiding this comment.
task_?
we shouldn't check null in side of the loop.
majetideepak
left a comment
majetideepak
left a comment
There was a problem hiding this comment.
@jkhaliqi Let's limit changes to Use of Uninitialized Variables
Please update the title. "[VL] Fix usage of uninitialized variables"
| velox::RowVectorPtr vector = nullptr; | ||
| while (true) { | ||
| auto future = velox::ContinueFuture::makeEmpty(); | ||
| if (task == nullptr) { |
There was a problem hiding this comment.
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()
| velox::memory::ScopedMemoryArbitrationContext ctx{}; | ||
| facebook::velox::exec::MemoryReclaimer::Stats status; | ||
| velox::memory::MemoryPool* pool; | ||
| facebook::velox::exec::MemoryReclaimer::Stats status{}; |
There was a problem hiding this comment.
This change is not required. facebook::velox::exec::MemoryReclaimer::Stats initializes its fields.
|
|
||
| Stats stats() const override { | ||
| Stats stats; // no-op | ||
| Stats stats{}; // no-op |
| VeloxMemoryManager::VeloxMemoryManager(const std::string& kind, std::unique_ptr<AllocationListener> listener) | ||
| : MemoryManager(kind), listener_(std::move(listener)) { | ||
| : MemoryManager(kind) { | ||
| if (listener == nullptr) { |
There was a problem hiding this comment.
Why add this check? Is it related to the CVE?
There was a problem hiding this comment.
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.
| namespace { | ||
| MemoryUsageStats collectVeloxMemoryUsageStats(const velox::memory::MemoryPool* pool) { | ||
| MemoryUsageStats stats; | ||
| if (pool == nullptr) { |
There was a problem hiding this comment.
No related to the CVE.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
removed as false positive.
| std::string subPlanPath = FilePathGenerator::getDataFilePath(file); | ||
|
|
||
| ::substrait::Plan substraitPlan; | ||
| ::substrait::Plan substraitPlan{}; |
There was a problem hiding this comment.
Use of Uninitialized Variable@cpp/velox/tests/Substrait2VeloxPlanValidatorTest.cc:45
| kHiveConnectorId, "hive_table", filterPushdownEnabled, connector::hive::SubfieldFilters{}, nullptr); | ||
| } else { | ||
| connector::hive::SubfieldFilters subfieldFilters; | ||
| connector::hive::SubfieldFilters subfieldFilters{}; |
| const ::substrait::WindowType& type, | ||
| const RowTypePtr& inputType) { | ||
| core::WindowNode::Frame frame; | ||
| core::WindowNode::Frame frame{}; |
| 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; |
There was a problem hiding this comment.
not required. std::optional is initialized by default.
There was a problem hiding this comment.
Use of Uninitialized Variable@cpp/velox/substrait/SubstraitToVeloxExpr.cc:318
if (!literalType.has_value()) {
figured i'd explicitly initialize as well
There was a problem hiding this comment.
Not required. The constructor of std::optional does this.
| case ::substrait::Expression_Literal::LiteralTypeCase::kEmptyList: | ||
| case ::substrait::Expression_Literal::LiteralTypeCase::kList: { | ||
| ArrayVectorPtr elements; | ||
| ArrayVectorPtr elements = nullptr; |
There was a problem hiding this comment.
not required. ArrayVectorPtr is a std::shared_ptr which is initialized by default.
Same below for RowVectorPtr and MapVectorPtr
There was a problem hiding this comment.
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?
jkhaliqi
force-pushed
the
jk_cve_variables
branch
2 times, most recently
from
c3b05d2 to
bdab7c5
Compare
jkhaliqi
changed the title
jkhaliqi
force-pushed
the
jk_cve_variables
branch
from
bdab7c5 to
53ccac0
Compare
|
|
||
| std::shared_ptr<ColumnarBatch> WholeStageResultIterator::next() { | ||
| tryAddSplitsToTask(); | ||
| if (task_ == nullptr) { |
There was a problem hiding this comment.
Can we throw exception instead of returning nullptr?
| return nullptr; | ||
| } | ||
| velox::RowVectorPtr vector; | ||
| velox::RowVectorPtr vector = nullptr; |
There was a problem hiding this comment.
redundant since RowVectorPtr is a shared pointer.
There was a problem hiding this comment.
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
| if (listener == nullptr) { | ||
| throw gluten::GlutenException("VeloxMemoryManager failed"); | ||
| } | ||
| listener_(std::move(listener)) |
There was a problem hiding this comment.
Why move this here?
You can keep it as is and check for listener_ above
|
|
||
| MemoryUsageStats collectGlutenAllocatorMemoryUsageStats(const MemoryAllocator* allocator) { | ||
| MemoryUsageStats stats; | ||
| MemoryUsageStats stats{}; |
| 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; |
There was a problem hiding this comment.
Not required. The constructor of std::optional does this.
| case ::substrait::Expression_Literal::LiteralTypeCase::kEmptyList: | ||
| case ::substrait::Expression_Literal::LiteralTypeCase::kList: { | ||
| ArrayVectorPtr elements; | ||
| ArrayVectorPtr elements = nullptr; |
jkhaliqi
force-pushed
the
jk_cve_variables
branch
3 times, most recently
from
4750239 to
41f2e96
Compare
Use of Uninitialized Variables
jkhaliqi
force-pushed
the
jk_cve_variables
branch
from
41f2e96 to
8db3e30
Compare
|
===== Performance report for TPCDS SF2000 with Velox backend, for reference only ====
|
|
===== Performance report for TPCH SF2000 with Velox backend, for reference only ====
|
fix security volation
5 participants
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)