Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion go/mysql/replication/mysql56_gtid_set.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,28 @@ 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.
Comment on lines +94 to +98

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Remove the unrequested explanatory allocation narrative

Unless the originating task explicitly requested source-level commentary, this newly added multi-paragraph rationale violates the repository’s strict instruction not to add explanatory comments; the machine- and implementation-specific allocation measurements also risk becoming stale while duplicating PR/commit context. Keep the implementation focused and move this narrative out of the source.

AGENTS.md reference: AGENTS.md:L20-L27

Useful? React with 👍 / 👎.

//
// The bound is a fraction of the remaining input rather than a constant, so
// 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)/2 + 1; nIntervals > max {
nIntervals = max
}
Comment on lines +100 to +114
intervals := make([]interval, 0, nIntervals)
for len(tail) > 0 {
if idx := strings.IndexByte(tail, ':'); idx >= 0 {
head = tail[:idx]
Expand Down
73 changes: 73 additions & 0 deletions go/mysql/replication/mysql56_gtid_set_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,10 @@ limitations under the License.
package replication

import (
"fmt"
"maps"
"reflect"
"runtime"
"strings"
"testing"

Expand Down Expand Up @@ -902,3 +904,74 @@ func TestSIDs(t *testing.T) {
require.Len(t, sids, 1)
assert.Equal(t, "8bc65cca-3fe4-11ed-bbfb-091034d48b3e", sids[0].String())
}

// 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)
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)
Comment on lines +924 to +928

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Make the cap-hint test exercise the clamp

This test passes unchanged against the parent implementation and therefore cannot guard the allocation fix: every generated :%d-%d interval occupies at least four bytes, so len(tail)/4+1 is never smaller than the colon-derived capacity and the new clamping branch is not taken. The colon-run test also passes before the fix because it checks only the existing parse error. Test the extracted capacity calculation or otherwise verify that malformed input receives bounded preallocation so reverting the fix makes a test fail.

AGENTS.md reference: AGENTS.md:L79-L80

Useful? React with 👍 / 👎.

}

// 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()
runtime.ReadMemStats(&before)
_, _ = ParseMysql56GTIDSet(hostile)
runtime.ReadMemStats(&after)
allocated := after.TotalAlloc - before.TotalAlloc
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)
}