When a subject with embedded nulls was inserted into an stree followe…

…d by another one with more nulls we could recurse infinitely and panic the server.

We now changed the no pivot to 127(DEL) and enforce that you can not insert a subject with that byte. Also make sure we do not recursively call into insert with no cpi movement.

This condition was from an old server that had corrupted the PSIM data (known issue fixed), but we were not detecting it, so added test for this when checking runes under isValidSubject.

Signed-off-by: Derek Collison <derek@nats.io>

server/filestore.go

@@ -1654,7 +1654,7 @@ func (fs *fileStore) recoverFullState() (rerr error) {
 				subj := buf[bi : bi+lsubj]
 				// We had a bug that could cause memory corruption in the PSIM that could have gotten stored to disk.
 				// Only would affect subjects, so do quick check.
-				if !isValidSubject(string(subj), true) {
+				if !isValidSubject(bytesToString(subj), true) {
 					os.Remove(fn)
 					fs.warn("Stream state corrupt subject detected")
 					return errCorruptState

server/stree/stree.go

@@ -55,6 +55,11 @@ func (t *SubjectTree[T]) Insert(subject []byte, value T) (*T, bool) {
 		return nil, false
 	}
 
+	// Make sure we never insert anything with a noPivot byte.
+	if bytes.IndexByte(subject, noPivot) >= 0 {
+		return nil, false
+	}
+
 	old, updated := t.insert(&t.root, subject, value, 0)
 	if !updated {
 		t.size++
@@ -151,7 +156,7 @@ func (t *SubjectTree[T]) insert(np *node, subject []byte, value T, si int) (*T,
 		ln.setSuffix(ln.suffix[cpi:])
 		si += cpi
 		// Make sure we have different pivot, normally this will be the case unless we have overflowing prefixes.
-		if p := pivot(ln.suffix, 0); si < len(subject) && p == subject[si] {
+		if p := pivot(ln.suffix, 0); cpi > 0 && si < len(subject) && p == subject[si] {
 			// We need to split the original leaf. Recursively call into insert.
 			t.insert(np, subject, value, si)
 			// Now add the update version of *np as a child to the new node4.

server/stree/stree_test.go

@@ -246,7 +246,7 @@ func TestSubjectTreeConstruction(t *testing.T) {
 	st.Insert(b("foo.bar"), 42)
 
 	checkNode := func(an *node, kind string, pors string, numChildren uint16) {
-		//		t.Helper()
+		t.Helper()
 		require_True(t, an != nil)
 		n := *an
 		require_True(t, n != nil)
@@ -258,7 +258,7 @@ func TestSubjectTreeConstruction(t *testing.T) {
 	checkNode(&st.root, "NODE4", "foo.ba", 2)
 	nn := st.root.findChild('r')
 	checkNode(nn, "NODE4", "r", 2)
-	checkNode((*nn).findChild(0), "LEAF", "", 0)
+	checkNode((*nn).findChild(noPivot), "LEAF", "", 0)
 	rnn := (*nn).findChild('.')
 	checkNode(rnn, "NODE4", ".", 3)
 	checkNode((*rnn).findChild('A'), "LEAF", "A", 0)
@@ -804,3 +804,41 @@ func TestSubjectTreeNilNoPanic(t *testing.T) {
 	_, found = st.Insert([]byte("foo"), 22)
 	require_False(t, found)
 }
+
+// This bug requires the trailing suffix contain repeating nulls \x00
+// and the second subject be longer with more nulls.
+func TestSubjectTreeInsertLongerLeafSuffixWithTrailingNulls(t *testing.T) {
+	st := NewSubjectTree[int]()
+	subj := []byte("foo.bar.baz_")
+	// add in 10 nulls.
+	for i := 0; i < 10; i++ {
+		subj = append(subj, 0)
+	}
+
+	st.Insert(subj, 1)
+	// add in 10 more nulls.
+	subj2 := subj
+	for i := 0; i < 10; i++ {
+		subj2 = append(subj, 0)
+	}
+	st.Insert(subj2, 2)
+
+	// Make sure we can look them up.
+	v, found := st.Find(subj)
+	require_True(t, found)
+	require_Equal(t, *v, 1)
+	v, found = st.Find(subj2)
+	require_True(t, found)
+	require_Equal(t, *v, 2)
+}
+
+// Make sure the system does not insert any subject with the noPivot (DEL) in it.
+func TestSubjectTreeInsertWithNoPivot(t *testing.T) {
+	st := NewSubjectTree[int]()
+	subj := []byte("foo.bar.baz.")
+	subj = append(subj, noPivot)
+	old, updated := st.Insert(subj, 22)
+	require_True(t, old == nil)
+	require_False(t, updated)
+	require_Equal(t, st.Size(), 0)
+}

server/stree/util.go

@@ -44,10 +44,14 @@ func copyBytes(src []byte) []byte {
 
 type position interface{ int | uint16 }
 
-// Can return 0 if we have all the subject as prefixes.
+// No pivot available.
+const noPivot = byte(127)
+
+// Can return 127 (DEL) if we have all the subject as prefixes.
+// We used to use 0, but when that was in the subject would cause infinite recursion in some situations.
 func pivot[N position](subject []byte, pos N) byte {
 	if int(pos) >= len(subject) {
-		return 0
+		return noPivot
 	}
 	return subject[pos]
 }

server/sublist.go

@@ -1181,6 +1181,10 @@ func isValidSubject(subject string, checkRunes bool) bool {
 		return false
 	}
 	if checkRunes {
+		// Check if we have embedded nulls.
+		if bytes.IndexByte(stringToBytes(subject), 0) >= 0 {
+			return false
+		}
 		// Since casting to a string will always produce valid UTF-8, we need to look for replacement runes.
 		// This signals something is off or corrupt.
 		for _, r := range subject {

server/sublist_test.go

@@ -634,6 +634,11 @@ func TestSublistValidSubjects(t *testing.T) {
 	checkBool(IsValidSubject("foo.>bar"), true, t)
 	checkBool(IsValidSubject("foo>.bar"), true, t)
 	checkBool(IsValidSubject(">bar"), true, t)
+
+	// Check for embedded nulls.
+	subj := []byte("foo.bar.baz.")
+	subj = append(subj, 0)
+	checkBool(isValidSubject(string(subj), true), false, t)
 }
 
 func TestSublistMatchLiterals(t *testing.T) {
@@ -697,7 +702,6 @@ func TestValidateDestinationSubject(t *testing.T) {
 	checkError(ValidateMappingDestination("foo.{{unknown(1)}}"), ErrInvalidMappingDestination, t)
 	checkError(ValidateMappingDestination("foo..}"), ErrInvalidMappingDestination, t)
 	checkError(ValidateMappingDestination("foo. bar}"), ErrInvalidMappingDestinationSubject, t)
-
 }
 
 func TestSubjectToken(t *testing.T) {