Skip to content
Open
Show file tree
Hide file tree
Changes from 3 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
15 changes: 14 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,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.
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 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Account for single-number intervals in the capacity bound

For accepted sets containing many bare intervals, such as sid:1:1:..., an interval plus its separator occupies only two bytes rather than the assumed four. The new maximum is therefore roughly half the required interval count, causing append to repeatedly grow and copy the slice and potentially making memory and CPU usage worse than the previous exact colon-count preallocation for this valid-input case. Base the bound on both supported interval forms or validate the input before using its exact interval count.

Useful? React with 👍 / 👎.

}
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
39 changes: 39 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 @@
package replication

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

Expand Down Expand Up @@ -902,3 +904,40 @@
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++ {

Check failure on line 920 in go/mysql/replication/mysql56_gtid_set_test.go

View workflow job for this annotation

GitHub Actions / Static Code Checks Etc

rangeint: for loop can be modernized using range over int (modernize)
// 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.
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),

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 Restore the cap bound before asserting its allocation ceiling

This revision adds the allocation assertion but does not include the production fix: ParseMysql56GTIDSet still preallocates strings.Count(tail, ":")+1 intervals, so this 1 MiB colon input necessarily allocates roughly 16 MiB for the slice and exceeds the 8 MiB limit. Consequently, this focused test—and therefore the package test suite—fails on the reviewed commit. Fresh evidence relative to the earlier comment is that the new assertion now exercises the regression, but the corresponding capacity clamp has disappeared from this revision.

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

Useful? React with 👍 / 👎.

"parsing %d colons allocated %d bytes for intervals that are all discarded",
1<<20, allocated)
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
Outdated
}
Loading