Skip to content

fix: clamp Substring negative offset by rune length, not byte length#909

Open
DucMinhNe wants to merge 1 commit into
samber:masterfrom
DucMinhNe:fix/substring-multibyte-negative-offset
Open

fix: clamp Substring negative offset by rune length, not byte length#909
DucMinhNe wants to merge 1 commit into
samber:masterfrom
DucMinhNe:fix/substring-multibyte-negative-offset

Conversation

@DucMinhNe

Copy link
Copy Markdown

What

Substring is documented and tested with rune (character) semantics, but the internal substring helper compares a negative offset against len(str), which is the byte length:

case offset < -len(str), offset == 0:

For an ASCII string byte length and rune length are equal, so this is fine. For a multibyte string the byte length is larger, so offsets in the band -byteLen < offset <= -runeLen fail this case, fall through to the count-from-the-end branch, walk -offset runes backwards, overshoot the start of the string and return a wrong or empty result.

The existing test Substring("hello", -10, 2) == "he" already pins the intended behaviour: a negative offset more negative than the length clamps to the start and takes length runes. Multibyte strings just were not covered.

Repro

lo.Substring("héllo", -6, 2) // got "h",  want "hé"
lo.Substring("héllo", -6, 1) // got "",   want "h"
lo.Substring("日本語", -4, 1)  // got "",   want "日"
lo.Substring("日本語", -5, 2)  // got "",   want "日本"

Fix

Compare against utf8.RuneCountInString(string(str)) instead of len(str). utf8 is already imported and the fallthrough from the positive-offset case is unaffected (a fallthrough ignores the case expression).

Added four assertions to TestSubstring covering the multibyte negative-offset band. go test ./... is green, gofmt/go vet clean.

substring() compared a negative offset against len(str), the byte length,
while Substring has rune/character semantics. For multibyte strings the
byte length exceeds the rune length, so offsets in the band
-byteLen < offset <= -runeLen skipped the clamp-to-start branch and fell
through to the count-from-end path, overshooting the front and returning a
wrong or empty result (e.g. Substring("héllo", -6, 2) returned "h" instead
of "hé"). Compare against utf8.RuneCountInString instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant