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
Closed

runtime: parse option numbers set through environment correctly #17718

martisch opened this issue Nov 1, 2016 · 3 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@martisch
Copy link
Contributor

martisch 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
Copy link

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
Copy link
Contributor Author

martisch 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
- 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 The path to resolution is known, but the work has not been done. label Nov 1, 2016
@quentinmit quentinmit added this to the Go1.8Maybe milestone Nov 1, 2016
@rsc
Copy link
Contributor

rsc 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 as completed 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.
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

4 participants