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

x/website: doc/tutorial/fuzz: conclusion: add warning to beware of missing inputs #69628

Open
westondan opened this issue Sep 25, 2024 · 3 comments
Labels
Documentation help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. website
Milestone

Comments

@westondan
Copy link

The Fuzz Conclusion falsely invites to infer that the Completed Code is "correct" or sufficiently tested: both are false.

In fact, the Fuzzer input range in this example is too limited to discover that the Reverse function deals very badly with multi-rune glyphs such as Combining Diacritical Marks that are important in non-English contexts. The worst part is that the code is not even broken: it does generate valid UTF8, just not what humans would expect, and would easily go unnoticed into production for months or years.

For example, the extant Reverse function would wrongly turn "LƐ̂DƐ̈GƆ̂BƆ̈" into "ƆB̂ƆG̈ƐD̂ƐL" (instead of the correct "Ɔ̈BƆ̂GƐ̈DƐ̂L"), erroneously moving the diacritics from the vowels to the consonants, but native English speaker SWEs (and most others as well) would be very hard pressed to know about this problem or include this test case among the hardwired inputs, and Fuzzer did not (after 30 sec) generate this kind of test input on its own.

At minimum, the Conclusion should warn the user to guard against a false sense of security, should suggest adding the above input as "extra credit" (being sure to prepend an ASCII to the orig string to guard against UTF8-valid but non-sensical initial combining rune inputs), and finally point the user towards testing an actual production-ready Reverse code, for example github.com/rivo/uniseg.ReverseString, for comparison.

@adonovan adonovan changed the title Should add warning in Conclusion to beware of what inputs may be missing. doc/tutorial/fuzz: conclusion: add warning to beware of missing inputs Sep 26, 2024
@mknyszek mknyszek changed the title doc/tutorial/fuzz: conclusion: add warning to beware of missing inputs x/website: conclusion: add warning to beware of missing inputs Sep 30, 2024
@mknyszek mknyszek changed the title x/website: conclusion: add warning to beware of missing inputs x/website: doc/tutorial/fuzz: conclusion: add warning to beware of missing inputs Sep 30, 2024
@gopherbot gopherbot added this to the Unreleased milestone Sep 30, 2024
@mknyszek
Copy link
Contributor

CC @golang/tools-team

@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 30, 2024
@hyangah
Copy link
Contributor

hyangah commented Oct 3, 2024

I don't see the tutorial claiming the final code shown there is bug free or understands language-specific restriction. However, explicit warnings may be helpful to avoid misinterpretation.

cc @golang/security for contents owner

@hyangah hyangah modified the milestones: Unreleased, Backlog Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. website
Projects
None yet
Development

No branches or pull requests

6 participants