Skip to content

Commit

Permalink
Fix VeloxReader Initialization Bug (facebookincubator#81)
Browse files Browse the repository at this point in the history
Summary:
Pull Request resolved: facebookincubator#81

The CLANGTIDY linter identified a use-after-move bug in the initialization of the `VeloxReader` executor, which was being initialized by a variable that had already been moved earlier in the initializer list. This can cause undefined behavior and in this case led to `barrier_` being given a null value, preventing the fieldReader from performing parallel reads.

After fixing this bug, some unit tests that used parallel reads started failing. Further investigation revealed that the executor in the `FieldReader` was being used incorrectly. Its executor depended on `bitmapPtr`, a pointer to a variable that went out of scope before the end of the executor contexts. This caused a stack-use-after-return exception, as described in this stack trace P1570849149.

This change addresses the incorrect initialization and the resulting exception.

Reviewed By: Yuhta

Differential Revision: D62285132

fbshipit-source-id: 4504c5c5fac97ba785fb66b18123d6c9af8278a5
  • Loading branch information
MacVincent Agha-Oko authored and facebook-github-bot committed Sep 9, 2024
1 parent 3fa84b2 commit 6a53405
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 6 deletions.
10 changes: 6 additions & 4 deletions dwio/nimble/velox/FieldReader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1846,22 +1846,24 @@ class RowFieldReader final : public FieldReader {
}
}

bits::Bitmap childrenBitmap{childrenBits, rowCount};
auto bitmapPtr = childrenBits ? &childrenBitmap : nullptr;

if (executor_) {
for (uint32_t i = 0; i < childrenReaders_.size(); ++i) {
auto& reader = childrenReaders_[i];
if (reader) {
executor_->add([selectedNonNullCount,
bitmapPtr,
childrenBits,
rowCount,
&reader,
&child = vector->childAt(i)]() {
bits::Bitmap childrenBitmap{childrenBits, rowCount};
auto bitmapPtr = childrenBits ? &childrenBitmap : nullptr;
reader->next(selectedNonNullCount, child, bitmapPtr);
});
}
}
} else {
bits::Bitmap childrenBitmap{childrenBits, rowCount};
auto bitmapPtr = childrenBits ? &childrenBitmap : nullptr;
for (uint32_t i = 0; i < childrenReaders_.size(); ++i) {
auto& reader = childrenReaders_[i];
if (reader) {
Expand Down
4 changes: 2 additions & 2 deletions dwio/nimble/velox/VeloxReader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -197,9 +197,9 @@ VeloxReader::VeloxReader(
: std::dynamic_pointer_cast<const velox::RowType>(
convertToVeloxType(*schema_))},
barrier_{
params.decodingExecutor
parameters_.decodingExecutor
? std::make_unique<velox::dwio::common::ExecutorBarrier>(
params.decodingExecutor)
parameters_.decodingExecutor)
: nullptr},
logger_{
parameters_.metricsLogger ? parameters_.metricsLogger
Expand Down

0 comments on commit 6a53405

Please sign in to comment.