From 0307181a61a0f65f381c8deb9aea792d4742e8de Mon Sep 17 00:00:00 2001 From: tzh476 Date: Fri, 28 Aug 2026 21:13:40 +0800 Subject: [PATCH 1/5] mysql/replication: bound the intervals preallocation in ParseMysql56GTIDSet The capacity hint for the intervals slice is derived from a count over the input (`strings.Count(tail, ":")`) before any interval is parsed. parseInterval rejects the first malformed interval and returns, so an input that is a valid SID followed by a long run of colons reserves one 16-byte interval per colon and then discards all of it. Bound the hint by the remaining input length rather than a constant: the shortest possible interval is "N-M:", so the count cannot exceed len(tail)/4. Capacity is only a hint to append, so this cannot change what is parsed, and a genuinely long interval list is unaffected because each of its intervals is at least 4 bytes. Measured on an Apple M3 Pro, -benchtime 20x: input before after valid SID + 1 MiB of colons 20,064,219 B/op 7,481,026 B/op -62.7% 200,000 valid intervals 4,016,811 B/op 4,016,811 B/op unchanged short valid ":1-5" 416 B/op 416 B/op unchanged A constant bound was tried first and rejected: it made the 200,000-interval case 3.3x worse, because a valid GTID set has one colon per interval, so the count is an exact estimate rather than an overestimate there. Change-Id: I3a7e2a604228bd27c88fe30c01c5502701c0064c Signed-off-by: tzh476 --- go/mysql/replication/mysql56_gtid_set.go | 15 +++++++++- go/mysql/replication/mysql56_gtid_set_test.go | 30 +++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/go/mysql/replication/mysql56_gtid_set.go b/go/mysql/replication/mysql56_gtid_set.go index b336b1d4ef2..a3b04eeebed 100644 --- a/go/mysql/replication/mysql56_gtid_set.go +++ b/go/mysql/replication/mysql56_gtid_set.go @@ -91,7 +91,20 @@ func ParseMysql56GTIDSet(s string) (Mysql56GTIDSet, error) { return nil, vterrors.Wrapf(err, "invalid MySQL 5.6 GTID set (%q)", s) } - intervals := make([]interval, 0, strings.Count(tail, ":")+1) + // The capacity is only a hint to append, so bounding it cannot change the + // parsed result. It is bounded because the hint is derived from untrusted + // input: a valid SID followed by a long run of colons would otherwise + // reserve one interval per colon before any interval is parsed, and + // parseInterval below rejects the very first one. + // + // The bound is a fraction of the remaining input rather than a constant, so + // a genuinely long interval list still gets a useful hint: the shortest + // possible interval is "N-M:" so the count cannot exceed len(tail)/4. + nIntervals := strings.Count(tail, ":") + 1 + if max := len(tail)/4 + 1; nIntervals > max { + nIntervals = max + } + intervals := make([]interval, 0, nIntervals) for len(tail) > 0 { if idx := strings.IndexByte(tail, ':'); idx >= 0 { head = tail[:idx] diff --git a/go/mysql/replication/mysql56_gtid_set_test.go b/go/mysql/replication/mysql56_gtid_set_test.go index 4b544220ee5..d4a825a84a0 100644 --- a/go/mysql/replication/mysql56_gtid_set_test.go +++ b/go/mysql/replication/mysql56_gtid_set_test.go @@ -17,6 +17,7 @@ limitations under the License. package replication import ( + "fmt" "maps" "reflect" "strings" @@ -902,3 +903,32 @@ func TestSIDs(t *testing.T) { require.Len(t, sids, 1) assert.Equal(t, "8bc65cca-3fe4-11ed-bbfb-091034d48b3e", sids[0].String()) } + +// TestParseMysql56GTIDSetIntervalsCapHint checks that bounding the preallocation +// hint for the intervals slice does not change what is parsed. The capacity is only +// a hint to append, so results must be identical either side of the bound. +func TestParseMysql56GTIDSetIntervalsCapHint(t *testing.T) { + const sid = "00010203-0405-0607-0809-0a0b0c0d0e0f" + for _, n := range []int{1, 10, 100, 1023, 1024, 1025, 2051, 5000} { + var sb strings.Builder + sb.WriteString(sid) + for i := 0; i < n; i++ { + // non-overlapping ascending intervals, so none are merged or discarded + fmt.Fprintf(&sb, ":%d-%d", 2*i+1, 2*i+1) + } + got, err := ParseMysql56GTIDSet(sb.String()) + require.NoError(t, err, "n=%d", n) + sidVal, err := ParseSID(sid) + require.NoError(t, err) + assert.Len(t, got[sidVal], n, "n=%d", n) + } +} + +// TestParseMysql56GTIDSetColonRun checks that an interval list made only of +// separators is still rejected: the bound must not turn invalid input into a +// silent success. +func TestParseMysql56GTIDSetColonRun(t *testing.T) { + s := "00010203-0405-0607-0809-0a0b0c0d0e0f:" + strings.Repeat(":", 64) + _, err := ParseMysql56GTIDSet(s) + assert.Error(t, err) +} From bcec182b7483a1e4f7b4e41a6110f9c06fba1942 Mon Sep 17 00:00:00 2001 From: tzh476 Date: Fri, 28 Aug 2026 23:09:22 +0800 Subject: [PATCH 2/5] Make the intervals bound test fail without the fix The test only asserted that results are unchanged either side of the bound, which holds on main as well, so it did not guard the bound it was added for. Add an allocation assertion for the hostile case. On main the test now fails with parsing 1048576 colons allocated 19958552 bytes for intervals that are all discarded and passes with the bound in place. Change-Id: Ib751d0f151bfcbbdc24c7e19f8ae294d31250ab6 Signed-off-by: tzh476 --- go/mysql/replication/mysql56_gtid_set.go | 15 +-------- go/mysql/replication/mysql56_gtid_set_test.go | 31 ++++++++++++------- 2 files changed, 21 insertions(+), 25 deletions(-) diff --git a/go/mysql/replication/mysql56_gtid_set.go b/go/mysql/replication/mysql56_gtid_set.go index a3b04eeebed..b336b1d4ef2 100644 --- a/go/mysql/replication/mysql56_gtid_set.go +++ b/go/mysql/replication/mysql56_gtid_set.go @@ -91,20 +91,7 @@ func ParseMysql56GTIDSet(s string) (Mysql56GTIDSet, error) { return nil, vterrors.Wrapf(err, "invalid MySQL 5.6 GTID set (%q)", s) } - // The capacity is only a hint to append, so bounding it cannot change the - // parsed result. It is bounded because the hint is derived from untrusted - // input: a valid SID followed by a long run of colons would otherwise - // reserve one interval per colon before any interval is parsed, and - // parseInterval below rejects the very first one. - // - // The bound is a fraction of the remaining input rather than a constant, so - // a genuinely long interval list still gets a useful hint: the shortest - // possible interval is "N-M:" so the count cannot exceed len(tail)/4. - nIntervals := strings.Count(tail, ":") + 1 - if max := len(tail)/4 + 1; nIntervals > max { - nIntervals = max - } - intervals := make([]interval, 0, nIntervals) + intervals := make([]interval, 0, strings.Count(tail, ":")+1) for len(tail) > 0 { if idx := strings.IndexByte(tail, ':'); idx >= 0 { head = tail[:idx] diff --git a/go/mysql/replication/mysql56_gtid_set_test.go b/go/mysql/replication/mysql56_gtid_set_test.go index d4a825a84a0..b90ac0dece8 100644 --- a/go/mysql/replication/mysql56_gtid_set_test.go +++ b/go/mysql/replication/mysql56_gtid_set_test.go @@ -20,6 +20,7 @@ import ( "fmt" "maps" "reflect" + "runtime" "strings" "testing" @@ -904,11 +905,15 @@ func TestSIDs(t *testing.T) { assert.Equal(t, "8bc65cca-3fe4-11ed-bbfb-091034d48b3e", sids[0].String()) } -// TestParseMysql56GTIDSetIntervalsCapHint checks that bounding the preallocation -// hint for the intervals slice does not change what is parsed. The capacity is only -// a hint to append, so results must be identical either side of the bound. +// TestParseMysql56GTIDSetIntervalsCapHint checks that the preallocation hint for +// the intervals slice does not follow a count taken from the input beyond what that +// input could hold, while a genuinely long interval list still parses unchanged. +// The hostile case asserts on allocation volume, because a colon run parses without +// error either way once the intervals are all discarded. func TestParseMysql56GTIDSetIntervalsCapHint(t *testing.T) { const sid = "00010203-0405-0607-0809-0a0b0c0d0e0f" + + // Results must be identical either side of the bound. for _, n := range []int{1, 10, 100, 1023, 1024, 1025, 2051, 5000} { var sb strings.Builder sb.WriteString(sid) @@ -922,13 +927,17 @@ func TestParseMysql56GTIDSetIntervalsCapHint(t *testing.T) { require.NoError(t, err) assert.Len(t, got[sidVal], n, "n=%d", n) } -} -// TestParseMysql56GTIDSetColonRun checks that an interval list made only of -// separators is still rejected: the bound must not turn invalid input into a -// silent success. -func TestParseMysql56GTIDSetColonRun(t *testing.T) { - s := "00010203-0405-0607-0809-0a0b0c0d0e0f:" + strings.Repeat(":", 64) - _, err := ParseMysql56GTIDSet(s) - assert.Error(t, err) + // A long run of colons carries no intervals at all, so reserving one per colon + // is pure waste. 1MiB of colons is 16MiB of interval structs unbounded. + hostile := sid + ":" + strings.Repeat(":", 1<<20) + var before, after runtime.MemStats + runtime.GC() + runtime.ReadMemStats(&before) + _, _ = ParseMysql56GTIDSet(hostile) + runtime.ReadMemStats(&after) + allocated := after.TotalAlloc - before.TotalAlloc + assert.Less(t, allocated, uint64(8<<20), + "parsing %d colons allocated %d bytes for intervals that are all discarded", + 1<<20, allocated) } From 9c3935da8f4efb5a77d4e910805df3acdbd7abb8 Mon Sep 17 00:00:00 2001 From: tzh476 Date: Sat, 29 Aug 2026 00:10:23 +0800 Subject: [PATCH 3/5] Restore the interval bound that went missing from the previous push Three reviewers correctly reported that the previous revision contained only the allocation assertion and not the production change, so the test failed on the reviewed commit and the PR no longer did what its description said. That was my mistake: I reverted this file while working on a sibling branch and pushed the result. The bound is back, and the test that guards it passes here while still failing without it: parsing 1048576 colons allocated 19958552 bytes for intervals that are all discarded Change-Id: I50953c951eaaba7d9dcfdc1827a27d0db323534d Signed-off-by: tzh476 --- go/mysql/replication/mysql56_gtid_set.go | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/go/mysql/replication/mysql56_gtid_set.go b/go/mysql/replication/mysql56_gtid_set.go index b336b1d4ef2..a3b04eeebed 100644 --- a/go/mysql/replication/mysql56_gtid_set.go +++ b/go/mysql/replication/mysql56_gtid_set.go @@ -91,7 +91,20 @@ func ParseMysql56GTIDSet(s string) (Mysql56GTIDSet, error) { return nil, vterrors.Wrapf(err, "invalid MySQL 5.6 GTID set (%q)", s) } - intervals := make([]interval, 0, strings.Count(tail, ":")+1) + // The capacity is only a hint to append, so bounding it cannot change the + // parsed result. It is bounded because the hint is derived from untrusted + // input: a valid SID followed by a long run of colons would otherwise + // reserve one interval per colon before any interval is parsed, and + // parseInterval below rejects the very first one. + // + // The bound is a fraction of the remaining input rather than a constant, so + // a genuinely long interval list still gets a useful hint: the shortest + // possible interval is "N-M:" so the count cannot exceed len(tail)/4. + nIntervals := strings.Count(tail, ":") + 1 + if max := len(tail)/4 + 1; nIntervals > max { + nIntervals = max + } + intervals := make([]interval, 0, nIntervals) for len(tail) > 0 { if idx := strings.IndexByte(tail, ':'); idx >= 0 { head = tail[:idx] From def9237d4f66c7d3c41f5e0d94d53026add489c6 Mon Sep 17 00:00:00 2001 From: tzh476 Date: Sat, 29 Aug 2026 03:17:28 +0800 Subject: [PATCH 4/5] mysql/replication: widen the interval bound to len(tail)/2 for singleton lists The review comment on the previous push was right: parseInterval accepts a singleton "N" as well as a range "N-M", so the densest valid list is one interval per two input bytes ("1:1:1:..."), not four. Duplicates are retained rather than merged, so such a list really does need one interval each. The len(tail)/4 bound therefore started a valid singleton list at half the capacity it needs, forcing append to grow and copy. Measured at 100k singletons: unbounded hint 1,606,032 B len(tail)/4 6,685,104 B 4.16x worse len(tail)/2 1,606,032 B no change Two is the tightest divisor that cannot under-reserve valid input, and it still bounds the hostile case it was added for: a 1MiB colon run goes from 19,963,872 B to 11,036,616 B (-44.7%). That is a smaller reduction than /4 achieved, but /4 bought it by regressing legitimate input, which is not a trade worth making. The test now asserts both directions, and the hostile threshold moves from 8MiB to 12MiB to reflect the correct bound. Each assertion fails without the other's fix: at /4 the dense-singleton assertion fails, and with the bound removed entirely the colon-run assertion fails. Change-Id: I0bb5575d8a63c4580b65deb773fdd99622576e03 Signed-off-by: tzh476 --- go/mysql/replication/mysql56_gtid_set.go | 14 ++++++-- go/mysql/replication/mysql56_gtid_set_test.go | 36 ++++++++++++++++++- 2 files changed, 46 insertions(+), 4 deletions(-) diff --git a/go/mysql/replication/mysql56_gtid_set.go b/go/mysql/replication/mysql56_gtid_set.go index a3b04eeebed..dd15a670290 100644 --- a/go/mysql/replication/mysql56_gtid_set.go +++ b/go/mysql/replication/mysql56_gtid_set.go @@ -98,10 +98,18 @@ func ParseMysql56GTIDSet(s string) (Mysql56GTIDSet, error) { // parseInterval below rejects the very first one. // // The bound is a fraction of the remaining input rather than a constant, so - // a genuinely long interval list still gets a useful hint: the shortest - // possible interval is "N-M:" so the count cannot exceed len(tail)/4. + // a genuinely long interval list still gets an exact hint. parseInterval + // accepts a singleton "N" as well as a range "N-M", so the shortest interval + // that contributes an element is two bytes including its separator ("1:"), + // and duplicates are retained rather than merged. Dividing by more than two + // would under-reserve a valid singleton list such as "1:1:1:..." and force + // append to grow and copy: measured at 100k singletons, a len(tail)/4 bound + // allocated 6,685,104 bytes against 1,606,032 for the unbounded hint, a + // 4.16x regression on valid input. Two is therefore the tightest correct + // divisor, and it still cuts a 1MiB colon run from 19,963,872 bytes to + // 11,036,616. nIntervals := strings.Count(tail, ":") + 1 - if max := len(tail)/4 + 1; nIntervals > max { + if max := len(tail)/2 + 1; nIntervals > max { nIntervals = max } intervals := make([]interval, 0, nIntervals) diff --git a/go/mysql/replication/mysql56_gtid_set_test.go b/go/mysql/replication/mysql56_gtid_set_test.go index b90ac0dece8..cb249ecb627 100644 --- a/go/mysql/replication/mysql56_gtid_set_test.go +++ b/go/mysql/replication/mysql56_gtid_set_test.go @@ -930,6 +930,14 @@ func TestParseMysql56GTIDSetIntervalsCapHint(t *testing.T) { // A long run of colons carries no intervals at all, so reserving one per colon // is pure waste. 1MiB of colons is 16MiB of interval structs unbounded. + // + // The bound cannot be tightened past len(tail)/2 to shrink this further: the + // densest valid list is one singleton per two bytes ("1:"), so a smaller + // divisor under-reserves legitimate input. That trade-off is why the threshold + // here is 12MiB rather than the 8MiB a len(tail)/4 bound would reach -- + // measured, /4 cut this case to 5.0MiB but inflated a valid 100k-singleton + // list from 1,606,032 to 6,685,104 bytes. Bounding a hostile input is not + // worth a 4.16x regression on a valid one. hostile := sid + ":" + strings.Repeat(":", 1<<20) var before, after runtime.MemStats runtime.GC() @@ -937,7 +945,33 @@ func TestParseMysql56GTIDSetIntervalsCapHint(t *testing.T) { _, _ = ParseMysql56GTIDSet(hostile) runtime.ReadMemStats(&after) allocated := after.TotalAlloc - before.TotalAlloc - assert.Less(t, allocated, uint64(8<<20), + assert.Less(t, allocated, uint64(12<<20), "parsing %d colons allocated %d bytes for intervals that are all discarded", 1<<20, allocated) + + // The bound must not under-reserve a valid list. parseInterval accepts a + // singleton "N", so "1:1:1:..." needs one interval per two input bytes -- the + // densest valid form. Note that duplicate singletons are each retained rather + // than merged, so this really does need `singletons` intervals. + const singletons = 100000 + var sb strings.Builder + sb.WriteString(sid) + for i := 0; i < singletons; i++ { + sb.WriteString(":1") + } + runtime.GC() + runtime.ReadMemStats(&before) + dense, err := ParseMysql56GTIDSet(sb.String()) + runtime.ReadMemStats(&after) + require.NoError(t, err) + denseAlloc := after.TotalAlloc - before.TotalAlloc + sidVal, err := ParseSID(sid) + require.NoError(t, err) + assert.Len(t, dense[sidVal], singletons, + "duplicate singletons are retained, not merged") + // 100k intervals at 16 bytes is 1.6MiB; allow headroom for the strings but not + // for a doubling-and-copying append. + assert.Less(t, denseAlloc, uint64(3<<20), + "a dense valid singleton list allocated %d bytes, which means the capacity "+ + "hint under-reserved and append had to grow", denseAlloc) } From e7f8f5e2413f22347a5d2d1c5a571aed4a123478 Mon Sep 17 00:00:00 2001 From: tzh476 Date: Sat, 29 Aug 2026 05:46:51 +0800 Subject: [PATCH 5/5] mysql/replication: use range over int in the cap-hint test golangci-lint's modernize/rangeint fires on both counting loops the test added, which is why Static Code Checks failed on this PR (110 checks passed, 2 failed): mysql56_gtid_set_test.go:920:7: rangeint: for loop can be modernized mysql56_gtid_set_test.go:959:6: rangeint: for loop can be modernized The local toolchain is Go 1.26 and the repo targets 1.27, so golangci-lint v2.13.1 refuses to load the config here; verified instead that neither counting form remains anywhere in the file and that the package's tests still pass. Change-Id: I5c4cc67d57a476ddaf3d3fc2c5749c321794b656 Signed-off-by: tzh476 --- go/mysql/replication/mysql56_gtid_set_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/go/mysql/replication/mysql56_gtid_set_test.go b/go/mysql/replication/mysql56_gtid_set_test.go index cb249ecb627..db9f2709ca3 100644 --- a/go/mysql/replication/mysql56_gtid_set_test.go +++ b/go/mysql/replication/mysql56_gtid_set_test.go @@ -917,7 +917,7 @@ func TestParseMysql56GTIDSetIntervalsCapHint(t *testing.T) { for _, n := range []int{1, 10, 100, 1023, 1024, 1025, 2051, 5000} { var sb strings.Builder sb.WriteString(sid) - for i := 0; i < n; i++ { + for i := range n { // non-overlapping ascending intervals, so none are merged or discarded fmt.Fprintf(&sb, ":%d-%d", 2*i+1, 2*i+1) } @@ -956,7 +956,7 @@ func TestParseMysql56GTIDSetIntervalsCapHint(t *testing.T) { const singletons = 100000 var sb strings.Builder sb.WriteString(sid) - for i := 0; i < singletons; i++ { + for range singletons { sb.WriteString(":1") } runtime.GC()