Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: check bounds using rune array instead of string len in substring #5043

Merged
merged 3 commits into from
Aug 3, 2022

Conversation

onelson
Copy link
Contributor

@onelson onelson commented Aug 3, 2022

Inputs that contain non-breaking spaces (NBSP), and assumedly a host of
other unicode characters caused strings.substring to panic when it
tried to index into an array of runes taken from a (seemingly larger)
string.

Failures looked like:

panic: runtime error: slice bounds out of range [:33] with capacity 32

This only seemed to panic when the inputs were "of a certain size" and
I'm not sure why that would be. Perhaps there are distinct thresholds
where the input causes the capacity to be just close enough to the
actual size of the rune count, but less than the string length in bytes.
Not sure.

The testcase added used an input seen in the wild and panicked prior to
the code change to substring.

Checklist

Dear Author 👋, the following checks should be completed (or explicitly dismissed) before merging.

  • ✏️ Write a PR description, regardless of triviality, to include the value of this PR
  • 🔗 Reference related issues
  • 🏃 Test cases are included to exercise the new code
  • 🧪 If new packages are being introduced to stdlib, link to Working Group discussion notes and ensure it lands under experimental/
  • 📖 If language features are changing, ensure docs/Spec.md has been updated

Dear Reviewer(s) 👋, you are responsible (among others) for ensuring the completeness and quality of the above before approval.

Inputs that contain non-breaking spaces (NBSP), and assumedly a host of
other unicode characters caused `strings.substring` to panic when it
tried to index into an array of runes taken from a (seemingly larger)
string.

Failures looked like:
```
panic: runtime error: slice bounds out of range [:33] with capacity 32
```

This only seemed to panic when the inputs were "of a certain size" and
I'm not sure why that would be. Perhaps there are distinct thresholds
where the input causes the capacity to be just close enough to the
actual size of the rune count, but less than the string length in bytes.
Not sure.

The testcase added used an input seen in the wild and panicked prior to
the code change to substring.
@onelson onelson marked this pull request as ready for review August 3, 2022 08:12
@onelson onelson requested a review from a team as a code owner August 3, 2022 08:12
@onelson onelson requested review from Rahul-Vasan and wolffcm and removed request for a team August 3, 2022 08:12
Copy link

@wolffcm wolffcm left a comment

Choose a reason for hiding this comment

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

Looks good. I had a comment about how this function is registered (it was a little inconsistent before you got here)

stdlib/strings/strings.go Outdated Show resolved Hide resolved
@onelson onelson merged commit 6121d7c into master Aug 3, 2022
@onelson onelson deleted the fix/substring-panic branch August 3, 2022 23:39
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.

2 participants