fix: secure temporary files and resource lookups - #19822
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens temporary file creation and classpath resource loading to address CodeQL findings across Druid’s test suite and several deep-storage segment pushers, while preserving a portable getconf lookup with a narrowly scoped suppression.
Changes:
- Replaced
File.createTempFileusages with test-managed temp locations (TemporaryFolder, JUnit5@TempDir) or DruidFileUtils.createTempDir, and improved cleanup paths. - Updated resource loading to use declaring-class literals and (in most places) close streams via try-with-resources.
- Switched several segment pushers and cache components to NIO temp file creation and ensured temp artifacts are deleted.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java | Uses declaring-class resource lookup and try-with-resources when copying resources to temp files. |
| server/src/test/java/org/apache/druid/segment/loading/LocalDataSegmentPullerTest.java | Uses JUnit-managed temp files instead of File.createTempFile. |
| server/src/test/java/org/apache/druid/metadata/input/SqlInputSourceTest.java | Replaces custom temp-dir tracking/cleanup with TemporaryFolder. |
| server/src/test/java/org/apache/druid/metadata/input/SqlEntityTest.java | Uses TemporaryFolder for temp files and closes file streams explicitly. |
| processing/src/test/java/org/apache/druid/segment/TestIndex.java | Uses FileUtils.createTempDir for merge/persist temp directories. |
| processing/src/test/java/org/apache/druid/segment/SchemalessIndexTest.java | Uses FileUtils.createTempDir for multiple temp directories used in index persistence/merge paths. |
| processing/src/test/java/org/apache/druid/segment/IndexMergerV9WithSpatialIndexTest.java | Switches temp dirs to FileUtils.createTempDir and consolidates cleanup to delete the root temp dir. |
| processing/src/test/java/org/apache/druid/segment/filter/SpatialFilterTest.java | Uses FileUtils.createTempDir and deleteOnExit for temp dirs used during persist/merge. |
| processing/src/test/java/org/apache/druid/segment/filter/SpatialFilterBonusTest.java | Same temp-dir hardening approach as SpatialFilterTest. |
| processing/src/test/java/org/apache/druid/segment/EmptyIndexTest.java | Uses FileUtils.createTempDir and deletes it in a finally block. |
| processing/src/test/java/org/apache/druid/query/DoubleStorageTest.java | Uses TemporaryFolder and passes an explicit index directory into helper method. |
| processing/src/test/java/org/apache/druid/java/util/common/io/smoosh/SmooshedFileMapperTest.java | Uses JUnit @TempDir folder-scoped files instead of global temp files. |
| processing/src/test/java/org/apache/druid/frame/FrameTest.java | Uses a temp directory per test and cleans it via Closer with FileUtils.deleteDirectory. |
| processing/src/test/java/org/apache/druid/data/input/impl/prefetch/JsonIteratorTest.java | Uses JUnit5 @TempDir for deterministic, test-managed temp files. |
| processing/src/main/java/org/apache/druid/java/util/metrics/CgroupCpuMonitor.java | Adds rationale and narrow CodeQL suppression for portable getconf PATH lookup. |
| extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3DataSegmentPusher.java | Uses NIO temp zip creation and ensures deletion in a finally block. |
| extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/cache/OffHeapNamespaceExtractionCacheManager.java | Uses NIO temp file creation for MapDB backing file. |
| extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/KinesisIndexTaskTest.java | Uses test-managed temp file for reports output. |
| extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/KafkaIndexTaskTest.java | Uses test-managed temp file for reports output. |
| extensions-core/hdfs-storage/src/test/java/org/apache/druid/storage/hdfs/HdfsDataSegmentPullerTest.java | Uses FileUtils.createTempDir and folder-scoped input file creation. |
| extensions-core/hdfs-storage/src/test/java/org/apache/druid/segment/loading/HdfsFileTimestampVersionFinderTest.java | Same temp-dir and input-file hardening as HDFS puller test. |
| extensions-core/google-extensions/src/main/java/org/apache/druid/storage/google/GoogleDataSegmentPusher.java | Uses NIO temp zip creation and deletes it in finally. |
| extensions-core/azure-extensions/src/main/java/org/apache/druid/storage/azure/AzureDataSegmentPusher.java | Uses NIO temp zip creation and deletes it in finally. |
| extensions-contrib/moving-average-query/src/test/java/org/apache/druid/query/movingaverage/MovingAverageQueryTest.java | Uses declaring-class resource lookup, non-null enforcement, and try-with-resources. |
| extensions-contrib/graphite-emitter/src/test/java/org/apache/druid/emitter/graphite/WhiteListBasedConverterTest.java | Switches to declaring-class resource lookup and test-managed temp file. |
| extensions-contrib/compressed-bigdecimal/src/test/java/org/apache/druid/compressedbigdecimal/aggregator/CompressedBigDecimalAggregatorTimeseriesTestBase.java | Closes resource streams and copies resources into temp files safely. |
| extensions-contrib/compressed-bigdecimal/src/test/java/org/apache/druid/compressedbigdecimal/aggregator/CompressedBigDecimalAggregatorGroupByTestBase.java | Closes resource streams and enforces non-null resource lookup. |
| extensions-contrib/cloudfiles-extensions/src/main/java/org/apache/druid/storage/cloudfiles/CloudFilesDataSegmentPusher.java | Uses NIO temp files for descriptor and zipped index creation. |
| extensions-contrib/cassandra-storage/src/main/java/org/apache/druid/storage/cassandra/CassandraDataSegmentPusher.java | Uses NIO temp zip creation, wraps upload in try, and always deletes the temp zip. |
| extensions-contrib/ambari-metrics-emitter/src/test/java/org/apache/druid/emitter/ambari/metrics/WhiteListBasedDruidToTimelineEventConverterTest.java | Switches to declaring-class resource lookup and test-managed temp file. |
| extensions-contrib/aliyun-oss-extensions/src/main/java/org/apache/druid/storage/aliyun/OssDataSegmentPusher.java | Uses NIO temp zip creation and ensures deletion in finally. |
| benchmarks/src/test/java/org/apache/druid/benchmark/indexing/IndexMergeBenchmark.java | Uses FileUtils.createTempDir and deletes the directory in finally. |
| benchmarks/src/test/java/org/apache/druid/benchmark/GenericIndexedBenchmark.java | Uses a folder-scoped metadata file instead of a global temp file. |
| benchmarks/src/test/java/org/apache/druid/benchmark/FrontCodedIndexedBenchmark.java | Uses folder-scoped metadata files instead of global temp files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 2 |
| P3 | 0 |
| Total | 2 |
Inspected all 34 changed files. The temporary-file/resource hardening is generally sound, but two cleanup blocks delete backing directories for live QueryableIndex instances; no other concrete findings.
Findings that could not be attached inline:
- processing/src/test/java/org/apache/druid/segment/IndexMergerV9WithSpatialIndexTest.java:266 - P2 Do not delete a live QueryableIndex backing directory.
loadIndex(tmpFile)returns an index backed by mapped files, but thisfinallyblock immediately deletestmpFile. The returned index is used later by parameterized tests; this can invalidate its backing files and fails on platforms that prohibit deleting open mapped files. Keep the directory alive until the index closes, such as with delete-on-exit or explicit test cleanup.
This is an automated review by Codex GPT-5.6-Sol
| FileUtils.deleteDirectory(secondFile); | ||
| FileUtils.deleteDirectory(thirdFile); | ||
| FileUtils.deleteDirectory(mergedFile); | ||
| FileUtils.deleteDirectory(tmpFile); |
There was a problem hiding this comment.
P2 Keep merged QueryableIndex files alive
makeMergedQueryableIndex returns a loaded merged index, then this finally block deletes the root containing the merged and input mapped indexes. Preserve these files until the returned index and segment are closed; immediate recursive deletion can invalidate the live mapped index and is not portable.
There was a problem hiding this comment.
Fixed in 51114329ab and pushed. makeQueryableIndex and makeMergedQueryableIndex now keep their mapped backing files available with deleteOnExit() instead of deleting the temporary roots in finally, so returned indexes remain usable across platforms. Focused IndexMergerV9WithSpatialIndexTest passed (36 tests), and Maven Checkstyle/PMD validation passed. Reviewed 34 of 34 changed files.
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
Reviewed 34 of 34 changed files. The prior live-index deletion issues are addressed; one test cleanup leak remains.
This is an automated review by Codex GPT-5.6-Sol
| File tmpFile = File.createTempFile("yay", "who"); | ||
| tmpFile.delete(); | ||
| final File tmpFile = FileUtils.createTempDir("spatial-index-merge"); | ||
| tmpFile.deleteOnExit(); |
There was a problem hiding this comment.
[P2] Recursively clean the persisted index trees
File.deleteOnExit() only deletes an empty path; it does not recursively delete directory contents. These roots and child directories contain persisted segment files, none of which are registered for deletion, so every shutdown deletion fails. The same issue occurs at line 260, causing each test JVM run to leave all generated spatial indexes under the system temp directory. Retain the indexes through the tests, then close them and call FileUtils.deleteDirectory from explicit class-level teardown.
What
getconfPATH lookup with a narrow CodeQL suppression and rationaleWhy
This addresses the 55 CodeQL warnings in the temporary-file and resource-lookup group:
java/local-temp-file-or-directory-information-disclosurejava/unsafe-get-resourcejava/relative-path-commandImpact
Temporary files are created with restrictive permissions or inside managed private directories. Resource lookup no longer depends on runtime subclass dispatch, and affected tests can load resources when packaged in JARs.
Root cause
The affected code used legacy
File.createTempFile, runtime-class resource lookup, and one intentionally portable executable lookup that CodeQL could not distinguish from an attacker-controlled command.Checks
git diff --checkCreated by GPT-5.6-Sol.