From 6a53405a2dc79537bae3ff93acd751388a83d568 Mon Sep 17 00:00:00 2001
From: MacVincent Agha-Oko <amacvincent@meta.com>
Date: Mon, 9 Sep 2024 13:45:54 -0700
Subject: [PATCH] Fix VeloxReader Initialization Bug (#81)

Summary:
Pull Request resolved: https://github.com/facebookincubator/nimble/pull/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
---
 dwio/nimble/velox/FieldReader.cpp | 10 ++++++----
 dwio/nimble/velox/VeloxReader.cpp |  4 ++--
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/dwio/nimble/velox/FieldReader.cpp b/dwio/nimble/velox/FieldReader.cpp
index 424f18c..e96fb02 100644
--- a/dwio/nimble/velox/FieldReader.cpp
+++ b/dwio/nimble/velox/FieldReader.cpp
@@ -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) {
diff --git a/dwio/nimble/velox/VeloxReader.cpp b/dwio/nimble/velox/VeloxReader.cpp
index 8835515..103472b 100644
--- a/dwio/nimble/velox/VeloxReader.cpp
+++ b/dwio/nimble/velox/VeloxReader.cpp
@@ -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