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

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

strconv: incorrect bit size in ParseComplex code #40706

benhoyt opened this issue Aug 11, 2020 · 4 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@benhoyt
Copy link
Contributor

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

CC @griesemer

@randall77
Copy link
Contributor

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 The path to resolution is known, but the work has not been done. label Aug 12, 2020
@gopherbot
Copy link

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

@benhoyt
Copy link
Contributor Author

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.

@golang golang locked and limited conversation to collaborators Oct 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants