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

Jump to bottom

strconv: ParseFloat panics on very long, valid inputs #2642

Closed
remyoudompheng opened this issue Dec 31, 2011 · 12 comments
Closed

strconv: ParseFloat panics on very long, valid inputs #2642

remyoudompheng opened this issue Dec 31, 2011 · 12 comments
Labels
FrozenDueToAge
Milestone
Go1

Comments

@remyoudompheng
Copy link
Contributor 

What steps will reproduce the problem?
1. Compile and run the following code:

package main

import (
    "fmt"
    "strconv"
)

func main() {
  var longnumber [4096]byte
  for i := range longnumber {
     longnumber[i] = '2'
  }
  longnumber[1] = '.'
  x, er := strconv.ParseFloat(string(longnumber[:]), 64)
  fmt.Println(x, er)
}

What is the expected output?

2.2222222222222222 <nil>

What do you see instead?

panic: runtime error: index out of range

goroutine 1 [running]:
strconv.(*decimal).set(0xf8400210d4, 0xf840024000, 0x1000, 0x0, 0xf840021100, ...)
    /opt/remy/go/src/pkg/strconv/atof.go:91 +0x315
----- stack segment boundary -----
strconv.atof64(0xf840024000, 0xf800001000, 0xf840021488, 0x0, 0x0, ...)
    /opt/remy/go/src/pkg/strconv/atof.go:383 +0xe0
strconv.ParseFloat(0xf840024000, 0x1000, 0xf800000040, 0x1000, 0x0, ...)
    /opt/remy/go/src/pkg/strconv/atof.go:432 +0x8d
main.main()
    /tmp/truc.go:14 +0xb9
----- stack segment boundary -----


Which compiler are you using (5g, 6g, 8g, gccgo)?

6g

Which operating system are you using?

linux

Which revision are you using?  (hg identify)

c956aa39a269+ (tip)

Please provide any additional information below.
@balasanjay
Copy link
Contributor

Comment 1:

The comment in decimal.go states "TODO(rsc): Can make d[] a bit smaller and add
truncated bool;". I'm actually curious why an array was used at all, instead of a slice
initialized to a smaller value and autoexpanding as necessary.
If that would be a valid solution, I could try writing a patch although I'll freely
admit I remember practically nothing about the representation of floating point values,
and it'll be a learning experience for me. As far as I can tell, should be pretty simple
just converting to a slice representation and changing to appends where necessary.
Sanjay

@rsc
Copy link
Contributor

rsc commented Jan 9, 2012

Comment 2:

The reason for the fixed buffer is so that there is just one
allocation (of the decimal struct), and with the new escape
analysis that is done on the stack, so it incurs no garbage
collection overhead.  I am surprised that adding another
allocation makes it faster.

@remyoudompheng
Copy link
Contributor Author

Comment 3:

Hum, i'm not sure what you are referring to. Is it CL
http://golang.org/cl/5503085/ ? 
In this CL I suggest using a *small* buffer in some cases, while still allowing for the
800-byte buffer. It is a bit ugly, but thanks to escape analysis, both are on the stack,
but only the one that comes into scope is zeroed (and zeroing 800 bytes is a significant
portion of the call).
However, escape analysis must be improved for this to work, which is proposed in CL
http://golang.org/cl/5489128/ . It would be great if you could have a look at
it, because I think it could have benefits in other places.

@remyoudompheng
Copy link
Contributor Author

Comment 4:

By the way, is there a way dynamic length make()'s that do not escape can be optimized
by escape analysis? I was thinking of compiling them like
if length < SomeConstant {
  slice some fixed size buffer on the stack
} else {
  call runtime·malloc
}

@robpike
Copy link
Contributor

robpike commented Jan 13, 2012

Comment 5:

Labels changed: added priority-go1.

@robpike
Copy link
Contributor

robpike commented Jan 13, 2012

Comment 6:

Status changed to Accepted.

@robpike
Copy link
Contributor

robpike commented Jan 13, 2012

Comment 7:

Owner changed to builder@golang.org.

@rsc
Copy link
Contributor

rsc commented Feb 6, 2012

Comment 8:

Remy, are you still planning to work on this?
I think it can be done by reading at most the
first 800 characters and then remembering 
whether one of the discarded characters was nonzero,
to force round-up mode.

@rsc
Copy link
Contributor

rsc commented Feb 6, 2012

Comment 9:

[now cc'ing remy]
Remy, are you still planning to work on this?
I think it can be done by reading at most the
first 800 characters and then remembering 
whether one of the discarded characters was nonzero,
to force round-up mode.

@remyoudompheng
Copy link
Contributor Author

Comment 10:

I may do so but not for the moment, I would have to get some time to get into it.

@rsc
Copy link
Contributor

rsc commented Feb 7, 2012

Comment 11:

Owner changed to @rsc.

Status changed to Started.

@rsc
Copy link
Contributor

rsc commented Feb 8, 2012

Comment 12:

This issue was closed by revision 3ee2085.

Status changed to Fixed.

@rsc rsc added this to the Go1 milestone Apr 10, 2015
@rsc rsc removed the priority-go1 label Apr 10, 2015
@golang golang locked and limited conversation to collaborators Jun 24, 2016
@rsc rsc removed their assignment Jun 22, 2022
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1
Development

No branches or pull requests
5 participants
@rsc @remyoudompheng @balasanjay @robpike @gopherbot