diff --git a/server/filestore.go b/server/filestore.go index b79ecbc9589..45e7ac17c82 100644 --- a/server/filestore.go +++ b/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 diff --git a/server/stree/stree.go b/server/stree/stree.go index d9167d94f7c..a289a629742 100644 --- a/server/stree/stree.go +++ b/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. diff --git a/server/stree/stree_test.go b/server/stree/stree_test.go index 7207e7e6910..2a84d3c7598 100644 --- a/server/stree/stree_test.go +++ b/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) +} diff --git a/server/stree/util.go b/server/stree/util.go index 585800aba0a..108f78fda92 100644 --- a/server/stree/util.go +++ b/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] } diff --git a/server/sublist.go b/server/sublist.go index 67eb88ae079..7171eef27ad 100644 --- a/server/sublist.go +++ b/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 { diff --git a/server/sublist_test.go b/server/sublist_test.go index beab4334129..c5718f8b0d9 100644 --- a/server/sublist_test.go +++ b/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) {