From af2c91d3b1d3f89b672609ce0acdafbee1c61993 Mon Sep 17 00:00:00 2001 From: Ben Hannel Date: Tue, 28 Jul 2026 16:25:24 +0000 Subject: [PATCH 1/2] fix(timezone): prevent alias lookup from overwriting existing cache entry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getTimezoneByName("US/Eastern") follows the alias link to "America/New_York" and unconditionally assigns a new shared_ptr into timezoneCache["America/New_York"], even when that entry already exists. The old shared_ptr refcount drops to zero inside timezone_mutex, destroying the LazyTimezone while TimestampColumnReader holds a raw Timezone* (writerTimezone_) that still references it. Reproduce without the fix (single-threaded, no sanitizer required): bazel test //c++/test:orc-test \ --test_filter='TimestampAliasCacheEviction*' \ --test_env=ASAN_OPTIONS=detect_leaks=0 # Expected under ASAN: heap-use-after-free in orc::TimestampColumnReader::next # Expected without ASAN: SIGSEGV on second rowReader->next() call Fix: guard the cache insertion with find() so alias lookups only populate a missing canonical entry and never replace an existing one. Adds TimestampAliasCacheEviction.readerSurvivesAliasCacheEviction in TestWriter.cc: writes an ORC file with writer timezone "America/New_York", reads the first batch (initializing writerTimezone_), then triggers the "US/Eastern" alias lookup to evict the cache entry, then reads the second batch — without the fix this dereferences freed memory. --- c++/src/Timezone.cc | 7 ++++- c++/test/TestWriter.cc | 60 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/c++/src/Timezone.cc b/c++/src/Timezone.cc index 6c3f58c079..b9499bd6d0 100644 --- a/c++/src/Timezone.cc +++ b/c++/src/Timezone.cc @@ -773,7 +773,12 @@ namespace orc { std::string newfilename(dir); newfilename += "/"; newfilename += it->second; - timezoneCache[newfilename] = std::make_shared(newfilename); + // Only insert if absent: overwriting an existing entry destroys the + // LazyTimezone while raw pointers in live TimestampColumnReaders still + // reference it, causing a use-after-free (SIGSEGV) on the next read. + if (timezoneCache.find(newfilename) == timezoneCache.end()) { + timezoneCache[newfilename] = std::make_shared(newfilename); + } timezoneCache[filename] = timezoneCache[newfilename]; } return *timezoneCache[filename].get(); diff --git a/c++/test/TestWriter.cc b/c++/test/TestWriter.cc index 5827bcdbca..be5fd6aa8e 100644 --- a/c++/test/TestWriter.cc +++ b/c++/test/TestWriter.cc @@ -2648,6 +2648,66 @@ namespace orc { } } + // Single-threaded crash test: a RowReader backed by a writer timezone that is + // later evicted from the alias-resolution cache must not segfault. + // + // Without the fix, getTimezoneByFilename() unconditionally overwrites the + // existing "America/New_York" cache entry when resolving the "US/Eastern" + // alias. The old shared_ptr refcount drops to zero inside timezone_mutex, + // freeing the LazyTimezone while TimestampColumnReader::writerTimezone_ (a + // raw Timezone*) still points to it. The second rowReader->next() call then + // dereferences freed memory: heap-use-after-free under ASAN or SIGSEGV. + // + // Reproduce without the fix: + // bazel test //c++/test:orc-test --test_filter='TimestampAliasCacheEviction*' \ + // --test_env=ASAN_OPTIONS=detect_leaks=0 + // Expected: heap-use-after-free in orc::TimestampColumnReader::next + TEST(TimestampAliasCacheEviction, readerSurvivesAliasCacheEviction) { + MemoryOutputStream memStream(DEFAULT_MEM_STREAM_SIZE); + MemoryPool* pool = getDefaultPool(); + std::unique_ptr type(Type::buildTypeFromString("struct")); + + // Write 2000 rows so that reading with a 1024-row batch requires two next() calls. + const uint64_t rowCount = 2000; + std::unique_ptr writer = + createWriter(64 * 1024 * 1024, 64 * 1024, 64 * 1024, CompressionKind_ZLIB, *type, pool, + &memStream, FileVersion::v_0_12(), 0, "America/New_York"); + std::unique_ptr batch = writer->createRowBatch(rowCount); + StructVectorBatch* structBatch = dynamic_cast(batch.get()); + TimestampVectorBatch* tsBatch = dynamic_cast(structBatch->fields[0]); + for (uint64_t i = 0; i < rowCount; ++i) { + tsBatch->data[i] = static_cast(i * 3600); + tsBatch->nanoseconds[i] = 0; + } + structBatch->numElements = rowCount; + tsBatch->numElements = rowCount; + writer->add(*batch); + writer->close(); + + auto inStream = std::make_unique(memStream.getData(), memStream.getLength()); + std::unique_ptr reader = createReader(pool, std::move(inStream)); + // GMT reader timezone != America/New_York writer timezone, so + // TimestampColumnReader::sameTimezone_ is false and writerTimezone_->getVariant() + // is called on every row in next(). + std::unique_ptr rowReader = createRowReader(reader.get(), "GMT"); + ASSERT_EQ(rowCount, reader->getNumberOfRows()); + + std::unique_ptr readBatch = rowReader->createRowBatch(1024); + + // First next() opens the stripe, constructs TimestampColumnReader, and stores + // writerTimezone_ = &getTimezoneByName("America/New_York"). + ASSERT_TRUE(rowReader->next(*readBatch)); + ASSERT_EQ(1024u, readBatch->numElements); + + // Without the fix: replaces timezoneCache["America/New_York"] with a new + // shared_ptr, drops the old refcount to zero, and frees the LazyTimezone + // that writerTimezone_ still points to. + (void)getTimezoneByName("US/Eastern"); + + // Without the fix: writerTimezone_->getVariant() dereferences freed memory. + EXPECT_TRUE(rowReader->next(*readBatch)); + } + std::vector testParams = {{FileVersion::v_0_11(), true}, {FileVersion::v_0_11(), false}, {FileVersion::v_0_12(), false}, From 189f3d2e1e653df00de22d810dd9481c4525ecc2 Mon Sep 17 00:00:00 2001 From: Ben Hannel Date: Tue, 28 Jul 2026 21:40:33 +0000 Subject: [PATCH 2/2] Simplify test --- c++/test/TestWriter.cc | 23 ++--------------------- 1 file changed, 2 insertions(+), 21 deletions(-) diff --git a/c++/test/TestWriter.cc b/c++/test/TestWriter.cc index be5fd6aa8e..764ead1c85 100644 --- a/c++/test/TestWriter.cc +++ b/c++/test/TestWriter.cc @@ -2648,20 +2648,6 @@ namespace orc { } } - // Single-threaded crash test: a RowReader backed by a writer timezone that is - // later evicted from the alias-resolution cache must not segfault. - // - // Without the fix, getTimezoneByFilename() unconditionally overwrites the - // existing "America/New_York" cache entry when resolving the "US/Eastern" - // alias. The old shared_ptr refcount drops to zero inside timezone_mutex, - // freeing the LazyTimezone while TimestampColumnReader::writerTimezone_ (a - // raw Timezone*) still points to it. The second rowReader->next() call then - // dereferences freed memory: heap-use-after-free under ASAN or SIGSEGV. - // - // Reproduce without the fix: - // bazel test //c++/test:orc-test --test_filter='TimestampAliasCacheEviction*' \ - // --test_env=ASAN_OPTIONS=detect_leaks=0 - // Expected: heap-use-after-free in orc::TimestampColumnReader::next TEST(TimestampAliasCacheEviction, readerSurvivesAliasCacheEviction) { MemoryOutputStream memStream(DEFAULT_MEM_STREAM_SIZE); MemoryPool* pool = getDefaultPool(); @@ -2686,9 +2672,6 @@ namespace orc { auto inStream = std::make_unique(memStream.getData(), memStream.getLength()); std::unique_ptr reader = createReader(pool, std::move(inStream)); - // GMT reader timezone != America/New_York writer timezone, so - // TimestampColumnReader::sameTimezone_ is false and writerTimezone_->getVariant() - // is called on every row in next(). std::unique_ptr rowReader = createRowReader(reader.get(), "GMT"); ASSERT_EQ(rowCount, reader->getNumberOfRows()); @@ -2699,12 +2682,10 @@ namespace orc { ASSERT_TRUE(rowReader->next(*readBatch)); ASSERT_EQ(1024u, readBatch->numElements); - // Without the fix: replaces timezoneCache["America/New_York"] with a new - // shared_ptr, drops the old refcount to zero, and frees the LazyTimezone - // that writerTimezone_ still points to. + // Populate aliases in the timezone cache (void)getTimezoneByName("US/Eastern"); - // Without the fix: writerTimezone_->getVariant() dereferences freed memory. + // Verify that writerTimezone_ is still a valid pointer EXPECT_TRUE(rowReader->next(*readBatch)); }