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: remove maxstring restriction when printing strings #16999

Closed
martisch opened this issue Sep 6, 2016 · 7 comments

Comments

Projects
None yet
6 participants
@martisch
Copy link
Member

commented Sep 6, 2016

go tip (go1.7 and many before)

in runtime/string.go there is a maxstrings variable that starts with a value of 256 and is updated whenever rawstring is called.

This limit can be expanded by concatenating any strings.

However it is not consistent with what the length of the longest go string really is:

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

package main

func main() {
    s := "012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789"+
         "012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789"+
         "012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789"+
         "012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789"
    _ = s+" "
    print(s)
}

e.g. defining a string constant that is longer than the starting maxstring value will not print.

when updating the maxstring length by e.g. adding the string with " " (or adding any other strings where the concatination is longer than orginal s) the above program prints the string fine.

Since gwrite later just truncates the string in a copy if it does not fit into the g.writebuf there seems no risk of out of bounds write. Since the limit can just be pushed upwards by arbitrary string concatenations there seems also to be no protection against the string output being cut off early depending on g.writebuf capacity.

Removing maxstring and running ./all.bash did not reveal any immediate problems.
( apart from an explicit test that maxstring is updated by gostringnocopy which would be obsolete once maxstring is removed )

rsc "I don't see that it's causing any harm, and it's still useful.":
https://groups.google.com/forum/#!msg/golang-dev/J2OiPumlwuc/neEqKcCWyf8J

however that seems to be from a time where runtime was not written in go and we did not have extra updates to maxstrings in e.g. gostringnocopy #8706 .

Would it be ok to remove maxstrings in runtime and maxstring updates from runtime/strings.go nowadays?

If not should cases were this protection against broken go string lengths is overprotecting be fixed (e.g. similar to /go/issues/8706) ?

/cc @josharian @randall77

@josharian

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2016

To be clear, is the motivation for removing it performance? code cleanliness?

I'm inclined towards removing it, but this is really not my call. Russ or Keith, maybe.

If we don't remove it, it'd be nice to fix printing of long constant strings.

@quentinmit quentinmit added this to the Go1.8 milestone Sep 6, 2016

@martisch

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2016

A bit of all:

  • it seems to be not documented what it is really protecting against, so i am not sure how useful it still is and if that couldn't be done differently then globally keeping maxstring up to date
  • if not removing it i would guess there are more places we need to take care of that maxstring is updated if a larger string is created that could be printed. ( Which is more difficult the more places there are were string([]byte) temporaries are made for the runtime without allocating a new string but just as a unsafe view on an existing byteslice )
  • performance would be a secondary concern (need to benchmark) but it is generally nice to have less globally synchronized variables if they can be easily replaced by other mechanisms.
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2016

I believe the original purpose of maxstring was to avoid printing enormous amounts of garbage if the runtime somehow got a garbage string.

Given that you've found a case where it is causing harm, I think we can reasonably remove it. It seems silly to go into contortions to preserve it.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Sep 7, 2016

@ianlancetaylor, that was @rsc's recommendation yesterday too.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2016

@bradfitz Thanks, I didn't see that.

@martisch

This comment has been minimized.

Copy link
Member Author

commented Sep 7, 2016

thanks for the feedback. I will go forward then and submit my local test CL to remove maxstring and maxstring updates after some final polish.

@gopherbot

This comment has been minimized.

Copy link

commented Sep 8, 2016

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

@gopherbot gopherbot closed this in 252093f Sep 8, 2016

@golang golang locked and limited conversation to collaborators Sep 8, 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.