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 panic in find_words due to string access outside of a character boundary #391

Merged
merged 1 commit into from
Jun 26, 2021

Conversation

MakotoE
Copy link
Contributor

@MakotoE MakotoE commented Jun 25, 2021

Closes #390

This fixes an attempt to access string bytes that may be within a character boundary.

Please check if this is actually how the code should be. All I know is that all tests pass.

@mgeisler
Copy link
Owner

Thanks! I'll double-check this when I'm home and then merge. We should really address #280 since I hope it could have cought this.

@MakotoE
Copy link
Contributor Author

MakotoE commented Jun 25, 2021

We should really address #280 since I hope it could have cought this.

I wouldn't run fuzz tests for all CI runs because you want commit tests to be fast. I think it should be run every night (on days when commits are merged). Or perhaps having a bot run it when requested in a PR might be good. There is a trade-off between being able to run fuzz tests for a long period of time occasionally or a short period of time frequently.

@mgeisler mgeisler changed the title Fix find_words() Fix panic in find_words due to string access outside of a character boundary Jun 26, 2021
@mgeisler mgeisler merged commit 747d698 into mgeisler:master Jun 26, 2021
@mgeisler
Copy link
Owner

Thanks for the PR, I'll make a 0.14.1 release with the fix now.

I wouldn't run fuzz tests for all CI runs because you want commit tests to be fast.

Yeah, I wouldn't want to prolong the tests by half or full hours. in this case, it turns out that the bug can be found by fuzzing for less than a minute. So I'll setup very brief fuzz testing on PRs, just to have a quick sanity check in the future.

@github-actions github-actions bot mentioned this pull request Jun 26, 2021
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.

Panic due to byte access between char boundary
2 participants