golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.73k stars 17.63k forks source link

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

Open westondan opened 1 month ago

westondan commented 1 month ago

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.

gabyhelp commented 1 month ago

Related Issues and Documentation

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

mknyszek commented 3 weeks ago

CC @golang/tools-team

hyangah commented 3 weeks ago

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