From 474b92608f7eabb57dcd89527d9405741663aff7 Mon Sep 17 00:00:00 2001 From: Yenda Li Date: Mon, 3 Feb 2025 21:44:05 -0800 Subject: [PATCH] fix: Avoid invalid access into random region in memory in OperatorUtils Summary: There are 2 bugs in processEncodedFilterResults: 1. We may call bits::isBitSet(values, index) and then rows.isValid(i), this means we may be passing in an invalid index to `bits::isBitSet(values, index)`. We need to call `rows.isValid(i)` before `bits::isBitSet(values, index)`. Because index may give a bogus value if it is not valid 2. We may access an invalid value in index via `auto index = indices[i];` if i is not valid. Fix is to check if the row is valid before processing Differential Revision: D69095863 --- velox/exec/OperatorUtils.cpp | 5 ++++- velox/exec/tests/OperatorUtilsTest.cpp | 12 ++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/velox/exec/OperatorUtils.cpp b/velox/exec/OperatorUtils.cpp index 136ef75da268..17d3d6d728bb 100644 --- a/velox/exec/OperatorUtils.cpp +++ b/velox/exec/OperatorUtils.cpp @@ -231,9 +231,12 @@ vector_size_t processEncodedFilterResults( auto* rawSelectedBits = filterEvalCtx.getRawSelectedBits(size, pool); memset(rawSelectedBits, 0, bits::nbytes(size)); for (int32_t i = 0; i < size; ++i) { + if (!rows.isValid(i)) { + continue; + } auto index = indices[i]; if ((!nulls || !bits::isBitNull(nulls, i)) && - bits::isBitSet(values, index) && rows.isValid(i)) { + bits::isBitSet(values, index)) { rawSelected[passed++] = i; bits::setBit(rawSelectedBits, i); } diff --git a/velox/exec/tests/OperatorUtilsTest.cpp b/velox/exec/tests/OperatorUtilsTest.cpp index 120c4347c10b..29e2b9202e4d 100644 --- a/velox/exec/tests/OperatorUtilsTest.cpp +++ b/velox/exec/tests/OperatorUtilsTest.cpp @@ -189,6 +189,18 @@ class OperatorUtilsTest : public OperatorTestBase { std::unique_ptr driverCtx_; }; +TEST_F(OperatorUtilsTest, processFilterResults) { + auto filteredResults = makeArrayVector({{1}}); + SelectivityVector filterRows(1); + filterRows.setValid(0, false); + filterRows.updateBounds(); + exec::FilterEvalCtx filterEvalCtx; + EXPECT_EQ( + exec::processFilterResults( + filteredResults, filterRows, filterEvalCtx, pool_.get()), + 0); +} + TEST_F(OperatorUtilsTest, wrapChildConstant) { auto constant = makeConstant(11, 1'000);