Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
30 changes: 30 additions & 0 deletions go/mysql/replication/mysql56_gtid_set_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package replication

import (
"fmt"
"maps"
"reflect"
"strings"
Expand Down Expand Up @@ -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)
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 👍 / 👎.

}
}

// 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)
}
Loading