fix: panic when JOIN condition type conversion fails (#23191)

XuPeng-SH · web-flow · commit b7167749c550 · 2025-12-02T10:04:34.000+08:00
### **User description** ## What type of PR is this? - [ ] API-change - [x] BUG - [ ] Improvement - [ ] Documentation - [ ] Feature - [ ] Test and CI - [ ] Code Refactoring ## Which issue(s) this PR fixes: issue matrixorigin/MO-Cloud#6733 ## What this PR does / why we need it: When a JOIN operation encounters a type mismatch (e.g., INT = VARCHAR), the type conversion fails during evalJoinCondition(), leaving HashmapBuilder.vecs in a partially initialized state. When Reset() is called during cleanup, it attempts to free nil pointers, causing a panic. ___ ### **PR Type** Bug fix, Tests ___ ### **Description** - Add nil pointer checks in `Reset()` and `Free()` methods to prevent panics - Introduce `cleanupPartiallyCreatedVecs()` helper to handle partial initialization failures - Call cleanup on errors in `evalJoinCondition()` to prevent memory leaks - Add comprehensive regression tests for nil pointer handling in Reset/Free - Add BVT test case for LEFT JOIN with type mismatch error handling ___ ### Diagram Walkthrough ```mermaid flowchart LR A["evalJoinCondition<br/>encounters error"] -->|calls| B["cleanupPartiallyCreatedVecs"] B -->|frees partial vecs| C["hb.vecs = nil"] C -->|prevents nil deref| D["Reset/Free safe"] E["Reset/Free methods"] -->|check nil| F["vecs[i] != nil"] F -->|check nil| G["vecs[i][j] != nil"] G -->|safe free| H["No panic"] ``` <details> <summary><h3> File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Bug fix</strong></td><td><table> <tr> <td> <details> <summary><strong>hashmap_util.go</strong><dd><code>Add nil pointer safety checks and cleanup helper</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> pkg/sql/colexec/hashmap_util/hashmap_util.go <ul><li>Add nil pointer checks in <code>Reset()</code> method for <code>hb.vecs</code> and <br><code>hb.UniqueJoinKeys</code> arrays<br> <li> Add nil pointer checks in <code>Free()</code> method for <code>hb.UniqueJoinKeys</code> array<br> <li> Introduce new <code>cleanupPartiallyCreatedVecs()</code> helper function to safely <br>free partially initialized vectors<br> <li> Call <code>cleanupPartiallyCreatedVecs()</code> in <code>evalJoinCondition()</code> when errors <br>occur during vector evaluation or duplication</ul> </details> </td> <td><a href="https://github.com/matrixorigin/matrixone/pull/23191/files#diff-4e472814399bbb0cac40fbad6d42daabca4111e950bebe1a2f5c4dbf8560bd74">+32/-4</a>&nbsp; &nbsp; </td> </tr> </table></td></tr><tr><td><strong>Tests</strong></td><td><table> <tr> <td> <details> <summary><strong>hashmap_util_test.go</strong><dd><code>Add comprehensive nil pointer regression tests</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> pkg/sql/colexec/hashmap_util/hashmap_util_test.go <ul><li>Add import for <code>vector</code> package<br> <li> Add <code>TestResetWithNilPointers()</code> regression test for nil pointer <br>handling in Reset with needDupVec=true<br> <li> Add <code>TestResetWithMixedNilAndValidPointers()</code> test for mixed nil and <br>valid vectors in Reset<br> <li> Add <code>TestFreeWithNilPointers()</code> regression test for nil pointer handling <br>in Free<br> <li> Add <code>TestFreeWithMixedNilAndValidPointers()</code> test for mixed nil and <br>valid vectors in Free</ul> </details> </td> <td><a href="https://github.com/matrixorigin/matrixone/pull/23191/files#diff-dc62011f0bc0fcb2eb8d9fe366c82ba5265a34cb8efc810e5a21b450763e1320">+95/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td> <details> <summary><strong>join.test</strong><dd><code>Add BVT test for LEFT JOIN type mismatch</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> test/distributed/cases/join/join.test <ul><li>Add BVT test case for LEFT JOIN with type mismatch (INT = VARCHAR)<br> <li> Test that type conversion error is returned instead of causing panic<br> <li> Include table creation, data insertion, and error validation</ul> </details> </td> <td><a href="https://github.com/matrixorigin/matrixone/pull/23191/files#diff-c786ef775fdeba111970aafa87ec9dc5c1086e298fc7cd6f3ae01896a2586b8b">+13/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td> <details> <summary><strong>join.result</strong><dd><code>Update test results with type mismatch error</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> test/distributed/cases/join/join.result <ul><li>Update expected output for existing JOIN test (whitespace formatting)<br> <li> Add expected error output for LEFT JOIN with type mismatch test case<br> <li> Include test cleanup statements for new test case</ul> </details> </td> <td><a href="https://github.com/matrixorigin/matrixone/pull/23191/files#diff-b989d5ea2130cdea874fcd458a930f745012401c8d4456f58ab1a51601e2106d">+11/-1</a>&nbsp; &nbsp; </td> </tr> </table></td></tr></tr></tbody></table> </details> ___
diff --git a/pkg/sql/colexec/hashmap_util/hashmap_util.go b/pkg/sql/colexec/hashmap_util/hashmap_util.go
@@ -115,8 +115,12 @@ func (hb *HashmapBuilder) Reset(proc *process.Process, hashTableHasNotSent bool)
 
 	if hb.needDupVec {
 		for i := range hb.vecs {
-			for j := range hb.vecs[i] {
-				hb.vecs[i][j].Free(proc.Mp())
+			if hb.vecs[i] != nil {
+				for j := range hb.vecs[i] {
+					if hb.vecs[i][j] != nil {
+						hb.vecs[i][j].Free(proc.Mp())
+					}
+				}
 			}
 		}
 	}
@@ -126,7 +130,9 @@ func (hb *HashmapBuilder) Reset(proc *process.Process, hashTableHasNotSent bool)
 	hb.StrHashMap = nil
 	hb.vecs = nil
 	for i := range hb.UniqueJoinKeys {
-		hb.UniqueJoinKeys[i].Free(proc.Mp())
+		if hb.UniqueJoinKeys[i] != nil {
+			hb.UniqueJoinKeys[i].Free(proc.Mp())
+		}
 	}
 	hb.UniqueJoinKeys = nil
 	hb.MultiSels.Free()
@@ -151,7 +157,9 @@ func (hb *HashmapBuilder) Free(proc *process.Process) {
 	hb.executors = nil
 	hb.vecs = nil
 	for i := range hb.UniqueJoinKeys {
-		hb.UniqueJoinKeys[i].Free(proc.Mp())
+		if hb.UniqueJoinKeys[i] != nil {
+			hb.UniqueJoinKeys[i].Free(proc.Mp())
+		}
 	}
 	hb.UniqueJoinKeys = nil
 }
@@ -168,18 +176,38 @@ func (hb *HashmapBuilder) FreeHashMapAndBatches(proc *process.Process) {
 	hb.Batches.Clean(proc.Mp())
 }
 
+// cleanupPartiallyCreatedVecs frees all vectors in hb.vecs that were successfully created.
+// This is used when an error occurs during evalJoinCondition to prevent memory leaks
+// and nil pointer dereferences in Reset.
+func (hb *HashmapBuilder) cleanupPartiallyCreatedVecs(proc *process.Process) {
+	for i := 0; i < len(hb.vecs); i++ {
+		if hb.vecs[i] != nil {
+			for j := 0; j < len(hb.vecs[i]); j++ {
+				if hb.vecs[i][j] != nil {
+					hb.vecs[i][j].Free(proc.Mp())
+				}
+			}
+		}
+	}
+	hb.vecs = nil
+}
+
 func (hb *HashmapBuilder) evalJoinCondition(proc *process.Process) error {
 	for idx1 := range hb.Batches.Buf {
 		tmpVes := make([]*vector.Vector, len(hb.executors))
 		hb.vecs = append(hb.vecs, tmpVes)
 		for idx2 := range hb.executors {
 			vec, err := hb.executors[idx2].Eval(proc, []*batch.Batch{hb.Batches.Buf[idx1]}, nil)
 			if err != nil {
+				// Clean up partially created vecs to prevent nil pointer issues in Reset
+				hb.cleanupPartiallyCreatedVecs(proc)
 				return err
 			}
 			if hb.needDupVec {
 				hb.vecs[idx1][idx2], err = vec.Dup(proc.Mp())
 				if err != nil {
+					// Clean up partially created vecs to prevent nil pointer issues in Reset
+					hb.cleanupPartiallyCreatedVecs(proc)
 					return err
 				}
 			} else {
diff --git a/pkg/sql/colexec/hashmap_util/hashmap_util_test.go b/pkg/sql/colexec/hashmap_util/hashmap_util_test.go
@@ -21,6 +21,7 @@ import (
 
 	"github.com/matrixorigin/matrixone/pkg/common/mpool"
 	"github.com/matrixorigin/matrixone/pkg/container/types"
+	"github.com/matrixorigin/matrixone/pkg/container/vector"
 	"github.com/matrixorigin/matrixone/pkg/pb/plan"
 	"github.com/matrixorigin/matrixone/pkg/testutil"
 	"github.com/stretchr/testify/require"
@@ -86,3 +87,97 @@ func TestHashMapAllocAndFree(t *testing.T) {
 	require.NoError(t, err)
 	hb.IntHashMap.Free()
 }
+
+// TestResetWithNilPointers tests that Reset() handles nil pointers gracefully
+// This is a regression test for the panic fix where Reset() would crash when
+// vecs or UniqueJoinKeys contained nil pointers.
+func TestResetWithNilPointers(t *testing.T) {
+	proc := testutil.NewProcessWithMPool(t, "", mpool.MustNewZero())
+	var hb HashmapBuilder
+
+	// Test case 1: vecs with nil pointers and needDupVec = true
+	hb.needDupVec = true
+	hb.vecs = make([][]*vector.Vector, 2)
+	hb.vecs[0] = make([]*vector.Vector, 2)
+	hb.vecs[1] = make([]*vector.Vector, 2)
+	// Set some vectors to nil to simulate partial initialization
+	hb.vecs[0][0] = nil
+	hb.vecs[0][1] = nil
+	hb.vecs[1][0] = nil
+	hb.vecs[1][1] = nil
+
+	// Test case 2: vecs with nil slice
+	hb.vecs = append(hb.vecs, nil)
+
+	// Test case 3: UniqueJoinKeys with nil pointers
+	hb.UniqueJoinKeys = make([]*vector.Vector, 3)
+	hb.UniqueJoinKeys[0] = nil
+	hb.UniqueJoinKeys[1] = nil
+	hb.UniqueJoinKeys[2] = nil
+
+	// Reset should not panic
+	hb.Reset(proc, true)
+	require.Nil(t, hb.vecs)
+	require.Nil(t, hb.UniqueJoinKeys)
+	require.Equal(t, int64(0), proc.Mp().CurrNB())
+}
+
+// TestResetWithMixedNilAndValidPointers tests Reset() with a mix of nil and valid vectors
+func TestResetWithMixedNilAndValidPointers(t *testing.T) {
+	proc := testutil.NewProcessWithMPool(t, "", mpool.MustNewZero())
+	var hb HashmapBuilder
+
+	// Create some valid vectors
+	vec1 := testutil.MakeInt32Vector([]int32{1, 2, 3}, nil)
+	vec2 := testutil.MakeInt32Vector([]int32{4, 5, 6}, nil)
+
+	// Test case: vecs with mix of nil and valid vectors
+	hb.needDupVec = true
+	hb.vecs = make([][]*vector.Vector, 2)
+	hb.vecs[0] = []*vector.Vector{vec1, nil}
+	hb.vecs[1] = []*vector.Vector{nil, vec2}
+
+	// Test case: UniqueJoinKeys with mix of nil and valid vectors
+	hb.UniqueJoinKeys = []*vector.Vector{vec1, nil, vec2}
+
+	// Reset should free valid vectors and not panic on nil
+	hb.Reset(proc, true)
+	require.Nil(t, hb.vecs)
+	require.Nil(t, hb.UniqueJoinKeys)
+	require.Equal(t, int64(0), proc.Mp().CurrNB())
+}
+
+// TestFreeWithNilPointers tests that Free() handles nil pointers gracefully
+func TestFreeWithNilPointers(t *testing.T) {
+	proc := testutil.NewProcessWithMPool(t, "", mpool.MustNewZero())
+	var hb HashmapBuilder
+
+	// Test case: UniqueJoinKeys with nil pointers
+	hb.UniqueJoinKeys = make([]*vector.Vector, 3)
+	hb.UniqueJoinKeys[0] = nil
+	hb.UniqueJoinKeys[1] = nil
+	hb.UniqueJoinKeys[2] = nil
+
+	// Free should not panic
+	hb.Free(proc)
+	require.Nil(t, hb.UniqueJoinKeys)
+	require.Equal(t, int64(0), proc.Mp().CurrNB())
+}
+
+// TestFreeWithMixedNilAndValidPointers tests Free() with a mix of nil and valid vectors
+func TestFreeWithMixedNilAndValidPointers(t *testing.T) {
+	proc := testutil.NewProcessWithMPool(t, "", mpool.MustNewZero())
+	var hb HashmapBuilder
+
+	// Create some valid vectors
+	vec1 := testutil.MakeInt32Vector([]int32{1, 2, 3}, nil)
+	vec2 := testutil.MakeInt32Vector([]int32{4, 5, 6}, nil)
+
+	// Test case: UniqueJoinKeys with mix of nil and valid vectors
+	hb.UniqueJoinKeys = []*vector.Vector{vec1, nil, vec2}
+
+	// Free should free valid vectors and not panic on nil
+	hb.Free(proc)
+	require.Nil(t, hb.UniqueJoinKeys)
+	require.Equal(t, int64(0), proc.Mp().CurrNB())
+}
diff --git a/test/distributed/cases/join/join.result b/test/distributed/cases/join/join.result
@@ -198,7 +198,7 @@ drop table if exists t2;
 create table t2 (a int);
 insert into t2 values(1);
 select * from (t1 join t2 on t1.a = t2.a);
-a    a   
+a    a
 1    1
 drop table if exists tt;
 drop table if exists tt2;
@@ -290,3 +290,13 @@ a    b    a    b
 42    3    42    1
 drop table x1;
 drop table x2;
+drop table if exists t1;
+drop table if exists t2;
+CREATE TABLE t1 (id INT);
+CREATE TABLE t2 (name VARCHAR(100));
+INSERT INTO t1 VALUES (1), (2);
+INSERT INTO t2 VALUES ('abc'), ('123');
+SELECT * FROM t1 LEFT JOIN t2 ON t1.id = t2.name;
+invalid argument cast to int, bad value abc
+drop table if exists t1;
+drop table if exists t2;
diff --git a/test/distributed/cases/join/join.test b/test/distributed/cases/join/join.test
@@ -201,3 +201,16 @@ select count(*) from x2 inner join x1 on x1.a = x2.a;
 select * from x2 inner join x1 on x1.a = x2.a order by x1.a limit 10;
 drop table x1;
 drop table x2;
+
+-- @case
+-- @desc:鲁棒性测试 - LEFT JOIN with type mismatch (INT = VARCHAR) should return error instead of panic
+-- @label:bvt
+drop table if exists t1;
+drop table if exists t2;
+CREATE TABLE t1 (id INT);
+CREATE TABLE t2 (name VARCHAR(100));
+INSERT INTO t1 VALUES (1), (2);
+INSERT INTO t2 VALUES ('abc'), ('123');
+SELECT * FROM t1 LEFT JOIN t2 ON t1.id = t2.name;
+drop table if exists t1;
+drop table if exists t2;
