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

all: re-review CL 220844 #42792

Closed
griesemer opened this issue Nov 24, 2020 · 8 comments
Closed

all: re-review CL 220844 #42792

griesemer opened this issue Nov 24, 2020 · 8 comments

Comments

@griesemer
Copy link
Contributor

@griesemer griesemer commented Nov 24, 2020

https://golang.org/cl/220844 introduced at least one regression (#42790). There may be more.

For instance, the change in https://go-review.googlesource.com/c/go/+/220844/3/src/bufio/bufio_test.go is at least questionable: It would be safer to write str += string(rune(i%26) + 'a') rather than the current str += string(rune(i)%26 + 'a').

@griesemer griesemer added this to the Go1.16 milestone Nov 24, 2020
@dmitshur dmitshur added the NeedsFix label Nov 24, 2020
@smasher164
Copy link
Member

@smasher164 smasher164 commented Nov 24, 2020

FWIW, @ianlancetaylor caught the pprof issue later, submitting https://golang.org/cl/221384.
src/runtime/pprof/internal/profile/proto.go src/internal/profile/proto.go can be checked off.

@smasher164
Copy link
Member

@smasher164 smasher164 commented Nov 24, 2020

Going to continue reviewing but here are some additional files in the CL we can check off:

src/runtime/string_test.go can also be checked off. It is only trying to use the integer value to offset from '0', so the conversion only exists to satisfy vet.
src/strings/strings_test.go can also be checked off. It is literally only converting utf8.RuneSelf, which is an untyped integer so it needs to satisfy the vet check.
src/encoding/xml/xml.go can also be checked off. The condition aims to check that some uint is <= unicode.MaxRune, before converting it to a string. Converting to a rune first is safe here.
src/strconv/quote_test.go can also be checked off. CanBackquote returns false on ASCII values < ' ' (32).
src/net/rpc/jsonrpc/all_test.go can also be checked off. The desired Id for the RPC response on the ith iteration of the loop is "\x00", "\x01", "\x02", ... which is exactly equivalent to string(i).

@smasher164
Copy link
Member

@smasher164 smasher164 commented Nov 25, 2020

src/bufio/bufio_test.go can be checked off but can also be made simpler to convert the entire expression inside the string conversion into a rune. The end goal of lines 144-152 in that CL is to produce an array of strings with the following contents (each line is an element of the array escaped):

"\n"
"a\n"
"ab\n"
"abc\n"
"abcd\n"
"abcde\n"
"abcdef\n"
"abcdefg\n"
"abcdefgh\n"
"abcdefghi\n"
"abcdefghij\n"
"abcdefghijk\n"
"abcdefghijkl\n"
"abcdefghijklm\n"
"abcdefghijklmn\n"
"abcdefghijklmno\n"
"abcdefghijklmnop\n"
"abcdefghijklmnopq\n"
"abcdefghijklmnopqr\n"
"abcdefghijklmnopqrs\n"
"abcdefghijklmnopqrst\n"
"abcdefghijklmnopqrstu\n"
"abcdefghijklmnopqrstuv\n"
"abcdefghijklmnopqrstuvw\n"
"abcdefghijklmnopqrstuvwx\n"
"abcdefghijklmnopqrstuvwxy\n"
"abcdefghijklmnopqrstuvwxyz\n"
"abcdefghijklmnopqrstuvwxyza\n"
"abcdefghijklmnopqrstuvwxyzab\n"
"abcdefghijklmnopqrstuvwxyzabc\n"
"\na\nab\nabc\nabcd\nabcde\nabcdef\nabcdefg\nabcdefgh\nabcdefghi\nabcdefghij\nabcdefghijk\nabcdefghijkl\nabcdefghijklm\nabcdefghijklmn\nabcdefghijklmno\nabcdefghijklmnop\nabcdefghijklmnopq\nabcdefghijklmnopqr\nabcdefghijklmnopqrs\nabcdefghijklmnopqrst\nabcdefghijklmnopqrstu\nabcdefghijklmnopqrstuv\nabcdefghijklmnopqrstuvw\nabcdefghijklmnopqrstuvwx\nabcdefghijklmnopqrstuvwxy\nabcdefghijklmnopqrstuvwxyz\nabcdefghijklmnopqrstuvwxyza\nabcdefghijklmnopqrstuvwxyzab\nabcdefghijklmnopqrstuvwxyzabc\n"

I tried all combinations for the string conversion, including string(i%26+'a'), string(rune(i)%26+'a'), string(rune(i%26)+'a')), and string(rune(i%26+'a')) and they all produce the same output.

@smasher164
Copy link
Member

@smasher164 smasher164 commented Nov 25, 2020

src/fmt/fmt_test.go can be checked off because string(0x110000) and string(rune(0x110000)) both represent the first invalid rune (utf8.MaxRune+1), which the comment above the testcase wants to test. Also, the loop at the top of Format is iterating through ASCII values to determine if they've been set in the printer state -- string(rune(i)) is safe here.

@griesemer
Copy link
Contributor Author

@griesemer griesemer commented Nov 25, 2020

Thanks, @smasher164, for re-reviewing these files. I agree with your assessment so far.

Remaining files:

  • scan.go looks correct
  • dnsclient_test.go is ok (testUniformity is called with sizes 2, 3, and 10, and rune(i) ranges from 0 to size-1)
  • value.go is incorrect: see #42835
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 25, 2020

Change https://golang.org/cl/273326 mentions this issue: reflect: fix Value.Convert for int-to-string conversions (regression)

gopherbot pushed a commit that referenced this issue Nov 26, 2020
The bug was introduced by https://golang.org/cl/220844.

Updates #42792.
Fixes #42835.

Change-Id: I03065c7526488aded35ef2f800b7162e1606877a
Reviewed-on: https://go-review.googlesource.com/c/go/+/273326
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 26, 2020

Change https://golang.org/cl/273526 mentions this issue: bufio: make string(int) conversion safer

gopherbot pushed a commit that referenced this issue Nov 28, 2020
Updates #42792.

Change-Id: I7e53426c41e5609d9dadceb300f7983ba7ad6577
Reviewed-on: https://go-review.googlesource.com/c/go/+/273526
Run-TryBot: Akhil Indurti <aindurti@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@smasher164
Copy link
Member

@smasher164 smasher164 commented Dec 2, 2020

Closing, now that CL 273326 and CL 273526 have gone in.

@smasher164 smasher164 closed this Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants