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

runtime: parse option numbers set through environment correctly #17718

Closed
martisch opened this issue Nov 1, 2016 · 3 comments

Comments

Projects
None yet
4 participants
@martisch
Copy link
Member

commented Nov 1, 2016

on go tip runtime.atoi does not do any kind of overflow checks or error reporting.
which can lead to internal variables being set to surprising numbers without a warning.

What did you do?

compile a test program:

package main

import "runtime"

func main() {
	print("runtime.MemProfileRate=", runtime.MemProfileRate, "\n")
}

https://play.golang.org/p/WhUyslK_Em

supply some number value flags:
$ GODEBUG=memprofilerate=10000000000000000000 ./test

What did you expect to see?

runtime.MemProfileRate=10000000000000000000
or an error
or a documented default value

What did you see instead?

runtime.MemProfileRate=-8446744073709551616

@gopherbot

This comment has been minimized.

Copy link

commented Nov 1, 2016

CL https://golang.org/cl/32390 mentions this issue.

@martisch martisch changed the title runtime: handle command line option numbers correctly runtime: parse option numbers set through environment correctly Nov 1, 2016

@martisch

This comment has been minimized.

Copy link
Member Author

commented Nov 1, 2016

https://golang.org/cl/32390 would fixes the atoi implementation and provides a ok return value such that call sites can report an error that the option could not be set correctly if need be.

gopherbot pushed a commit that referenced this issue Nov 1, 2016

runtime: improve atoi implementation
- Adds overflow checks
- Adds parsing of negative integers
- Adds boolean return value to signal parsing errors
- Adds atoi32 for parsing of integers that fit in an int32
- Adds tests

Handling of errors to provide error messages
at the call sites is left to future CLs.

Updates #17718

Change-Id: I3cacd0ab1230b9efc5404c68edae7304d39bcbc0
Reviewed-on: https://go-review.googlesource.com/32390
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

@quentinmit quentinmit added the NeedsFix label Nov 1, 2016

@quentinmit quentinmit added this to the Go1.8Maybe milestone Nov 1, 2016

@rsc

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2016

Thanks @martisch. The CL you submitted fixes the issue as far as I am concerned. A bad setting in the environment is not cause for failing to run the program.

@rsc rsc closed this Nov 2, 2016

@golang golang locked and limited conversation to collaborators Nov 2, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.