diff --git a/PR.md b/PR.md new file mode 100644 index 0000000..1dfcdc4 --- /dev/null +++ b/PR.md @@ -0,0 +1,64 @@ +# fix(strutil): prevent Substr panic on out-of-range negative length + +## Problem + +`strutil.Substr` panics with a slice-bounds error when a negative length trims +more characters than remain after `pos`: + +```go +strutil.Substr("hello", 4, -3) // panic: slice bounds out of range [4:2] +strutil.Substr("hello", 3, -5) // panic +strutil.Substr("abcDEF123", 8, -3) // panic +``` + +A negative `length` means "stop that many runes before the end of the string" +(this is the documented, tested behaviour, e.g. `Substr("abcDEF123", 2, -3) == +"cDEF"`). When the resulting stop index falls before `pos`, the final slice +`runes[pos:stopIdx]` has `low > high` and panics. + +## Root cause + +`strutil/split.go`, `Substr`: + +```go +stopIdx := pos + length +if length == 0 || stopIdx > strLn { + stopIdx = strLn +} else if length < 0 { + stopIdx = strLn + length // can become < pos +} +return string(runes[pos:stopIdx]) +``` + +For example `Substr("hello", 4, -3)`: `strLn == 5`, `pos == 4`, so +`stopIdx = 5 + (-3) = 2`, and `runes[4:2]` panics. + +## Fix + +Clamp `stopIdx` to `pos` so an over-long negative trim yields an empty string +instead of panicking: + +```go +if stopIdx < pos { + stopIdx = pos +} +``` + +This is consistent with the existing out-of-range guard `if pos >= strLn` which +also returns an empty string. + +## Test + +Extended `TestSubstr` in `strutil/split_test.go` with the three panicking cases, +each asserting an empty-string result. + +- Gate: `go test ./strutil/ -run 'TestSubstr$'` -> ok (PASS) +- `go vet ./strutil/` -> exit 0 +- `go build ./...` -> exit 0 + +Counterfactual: reverting the clamp makes `TestSubstr` panic with +`slice bounds out of range [4:2]`. + +Note: `TestDateSNv3/dateLen4_base62` in `strutil/gensn_test.go` is flaky and +already fails on the unmodified base commit (it asserts uniqueness of randomly +generated serial numbers); it is unrelated to this change. diff --git a/strutil/split.go b/strutil/split.go index 4961b9b..d41dc60 100644 --- a/strutil/split.go +++ b/strutil/split.go @@ -166,6 +166,12 @@ func Substr(s string, pos, length int) string { stopIdx = strLn + length } + // A negative length can move stopIdx before pos when it trims more than + // the remaining substring; clamp to pos to avoid an out-of-range slice. + if stopIdx < pos { + stopIdx = pos + } + return string(runes[pos:stopIdx]) } diff --git a/strutil/split_test.go b/strutil/split_test.go index 2a97caf..4e57ee7 100644 --- a/strutil/split_test.go +++ b/strutil/split_test.go @@ -120,6 +120,12 @@ func TestSubstr(t *testing.T) { assert.Eq(t, "", strutil.Substr("abcDEF", 23, 5)) assert.Eq(t, "cDEF12", strutil.Substr("abcDEF123", 2, -1)) assert.Eq(t, "cDEF", strutil.Substr("abcDEF123", 2, -3)) + + // negative length trimming more than the remaining substring must not + // panic; it should yield an empty string. + assert.Eq(t, "", strutil.Substr("hello", 4, -3)) + assert.Eq(t, "", strutil.Substr("hello", 3, -5)) + assert.Eq(t, "", strutil.Substr("abcDEF123", 8, -3)) } func TestSplitInlineComment(t *testing.T) {