From 674cfe7d65d80baab3287a0bd699fcb3410b2843 Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Fri, 26 Jun 2026 15:03:13 +0800 Subject: [PATCH 1/2] [GLUTEN-12378][CORE] Return a defensive copy from ConsistentHash.getPartition() getPartition() returned the internal partition set directly, so a caller could mutate the ring's state through it and the reference escaped after the read lock was released. Every other accessor (e.g. getNodes()) already copies. Return a defensive copy for consistency and to keep the @ThreadSafe contract intact. Add a test that mutating the returned set does not affect the ring. --- .../org/apache/gluten/hash/ConsistentHash.java | 5 ++++- .../org/apache/gluten/hash/ConsistentHashTest.java | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/gluten-core/src/main/java/org/apache/gluten/hash/ConsistentHash.java b/gluten-core/src/main/java/org/apache/gluten/hash/ConsistentHash.java index b432049645e..b8dd8844a2a 100644 --- a/gluten-core/src/main/java/org/apache/gluten/hash/ConsistentHash.java +++ b/gluten-core/src/main/java/org/apache/gluten/hash/ConsistentHash.java @@ -162,7 +162,10 @@ public Set getNodes() { public Set> getPartition(T node) { lock.readLock().lock(); try { - return nodes.get(node); + // Return a defensive copy: the map value is internal mutable state, and getNodes() copies + // for the same reason. Callers get a snapshot they can't use to mutate the ring. + Set> partitions = nodes.get(node); + return partitions == null ? null : new HashSet<>(partitions); } finally { lock.readLock().unlock(); } diff --git a/gluten-core/src/test/java/org/apache/gluten/hash/ConsistentHashTest.java b/gluten-core/src/test/java/org/apache/gluten/hash/ConsistentHashTest.java index 875fc460823..aacaade84d2 100644 --- a/gluten-core/src/test/java/org/apache/gluten/hash/ConsistentHashTest.java +++ b/gluten-core/src/test/java/org/apache/gluten/hash/ConsistentHashTest.java @@ -163,6 +163,20 @@ public String key() { }); } + @Test + public void testGetPartitionReturnsDefensiveCopy() { + HostNode node = new HostNode("executor-1"); + consistentHash.addNode(node); + + Set> partitions = + consistentHash.getPartition(node); + Assert.assertEquals(REPLICAS, partitions.size()); + + // Mutating the returned set must not affect the ring's internal state. + partitions.clear(); + Assert.assertEquals(REPLICAS, consistentHash.getPartition(node).size()); + } + private static class HostNode implements ConsistentHash.Node { private final String host; From 12f31b9a7ae853ab525b2647cb0ac1616617b7ba Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Fri, 26 Jun 2026 15:10:37 +0800 Subject: [PATCH 2/2] [GLUTEN-12378][CORE] Make Partition.setSlot non-public so getPartition() is a safe snapshot The defensive copy stops callers from mutating the returned set, but the Partition elements were still shared and setSlot() was public, so a caller could change a slot and corrupt the ring (e.g. removeNode() would then drop the wrong entry). Only add() assigns slots during construction, so make setSlot private (accessible as a nestmate). --- .../main/java/org/apache/gluten/hash/ConsistentHash.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/gluten-core/src/main/java/org/apache/gluten/hash/ConsistentHash.java b/gluten-core/src/main/java/org/apache/gluten/hash/ConsistentHash.java index b8dd8844a2a..31fdebe6c67 100644 --- a/gluten-core/src/main/java/org/apache/gluten/hash/ConsistentHash.java +++ b/gluten-core/src/main/java/org/apache/gluten/hash/ConsistentHash.java @@ -258,7 +258,11 @@ public T getNode() { return node; } - public void setSlot(long slot) { + // Non-public on purpose: only ConsistentHash.add() assigns the slot, during ring construction + // (accessible as a nestmate). Keeping it out of the public API stops callers of getPartition() + // from mutating ring state through a returned Partition (e.g. changing a slot so removeNode() + // drops the wrong entry). + private void setSlot(long slot) { this.slot = slot; }