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

strconv: incorrect bit size in ParseComplex code #40706

Open
benhoyt opened this issue Aug 11, 2020 · 4 comments
Open

strconv: incorrect bit size in ParseComplex code #40706

benhoyt opened this issue Aug 11, 2020 · 4 comments
Assignees
Labels
Milestone

Comments

@benhoyt
Copy link
Contributor

@benhoyt benhoyt commented Aug 11, 2020

This is not a bug, but a mistake in the code that made me think "how does this work?", and could be a bug waiting to happen. In the code for strconv.ParseComplex() (added in Go 1.15), it sets size to 128 by default (instead of 64), and passes that size to parseFloatPrefix. That function should really receive 64 for this case, but it currently happens to work with anything that's not 32, so this doesn't actually cause a bug. Here's the code for ParseComplex:

func ParseComplex(s string, bitSize int) (complex128, error) {
	size := 128
	if bitSize == 64 {
		size = 32 // complex64 uses float32 parts
	}
        // ...
	re, n, err := parseFloatPrefix(s, size)
        // ...
}

And parseFloatPrefix is defined as follows -- it handles anything other than bitSize == 32 as 64, so the invalid size of 128 happens to work:

func parseFloatPrefix(s string, bitSize int) (float64, int, error) {
	if bitSize == 32 {
		f, n, err := atof32(s)
		return float64(f), n, err
	}
	return atof64(s)
}

So the minimal change would be to change the size := 128 to size := 64 in ParseComplex. But there were a couple of other things I saw while in there:

  • There don't seem to be any tests for ParseComplex with bitSize == 64 (two float32's): https://golang.org/src/strconv/atoc_test.go -- it doesn't seem good that we're not testing the code paths for each size. many of the bitSize 128 tests should work the same (and we could reuse those), but the range/overflow ones will need to be different.
  • There don't seem to be any tests at all for FormatComplex. I know it's pretty trivially defined in terms of FormatFloat, but there is some logic in it, and we should still have tests to avoid regressions.
  • It seems to me that parseFloatPrefix (and ParseFloat) should panic if bitSize is anything other than 64 or 128. FormatComplex already does this (code here), and so it seems for consistency (and to catch issues like this one) the other functions should do similar checks. ParseFloat and FormatFloat are clearly documented to only allow 32 and 64. Alternatively they could return an error for this case, but it seems to me errors should only be returned for bad input, not invalid types, so the panic is the right thing to do.

If folks agree, I'd be happy to put up a CL for these changes.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 11, 2020

@randall77
Copy link
Contributor

@randall77 randall77 commented Aug 11, 2020

Fixing ParseComplex sounds fine.

I think it would be strange for ParseFloat to panic when it has an error return specified. Reporting errors in two different ways would just lead to confusion.
FormatFloat would need to panic, though (or return a "bad bit size" string).

More tests are always welcome.

@griesemer griesemer self-assigned this Aug 12, 2020
@griesemer griesemer added this to the Go1.16 milestone Aug 12, 2020
@griesemer griesemer added the NeedsFix label Aug 12, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Aug 14, 2020

Change https://golang.org/cl/248219 mentions this issue: strconv: fix incorrect bit size in ParseComplex; add tests

@benhoyt
Copy link
Contributor Author

@benhoyt benhoyt commented Aug 14, 2020

@griesemer Thanks. I'm not sure how the Go project uses GitHub "assignees" -- but I'm hoping the fact you assigned yourself doesn't mean I can't submit a CL. :-) So I've submitted https://go-review.googlesource.com/c/go/+/248219/ with this fix and the addition of some tests -- see what you think. Details in commit message.

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
5 participants
You can’t perform that action at this time.