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
internal/fmtsort: restrict the for-range by len(key) #33277
Conversation
this modification prevents the fmt panicking with reason index out of range Fixes golang#33275 Change-Id: I81fbd1997b8cd1f5f30549eb4640b516105d1dcc
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
i signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
This PR (HEAD: 57a206b) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/187577 to see it. Tip: You can toggle comments from me using the |
Message from Gobot Gobot: Patch Set 1: Congratulations on opening your first change. Thank you for your contribution! Next steps: Most changes in the Go project go through a few rounds of revision. This can be During May-July and Nov-Jan the Go project is in a code freeze, during which Please don’t reply on this GitHub thread. Visit golang.org/cl/187577. |
Message from Martin Möhrmann: Patch Set 2: Code-Review-1 Thanks but please discuss this on the issue tracker first. This wont fix the inherent problem of a race when concurrently writing to the map while iterating over it. And if there are races of unsynchronised reading and writing then fmtsort does not need to protect against them as other hazards can still appear until there is proper synchronisation. Please don’t reply on this GitHub thread. Visit golang.org/cl/187577. |
Message from Keith Randall: Patch Set 2: Code-Review+1 I'm fine with this CL. It can only be triggered by a data race, but if there's something simple we can do to make the error better, we should. The other options would be to:
Please don’t reply on this GitHub thread. Visit golang.org/cl/187577. |
Message from Martin Möhrmann: Patch Set 2:
I would argue the better improvement here (if the panic should be avoided) would be to use append to add items to the slice as this will both handle more and less items than expected. We probably should add a comment why this is programmed defensively so it is not optimised away by someone else later. But I would rather as suggested add some marker/panic here that a race is observed than just avoiding the index panic. Please don’t reply on this GitHub thread. Visit golang.org/cl/187577. |
53bd915
to
6139019
Compare
4a7ed1f
to
0f992b9
Compare
Message from Brad Fitzpatrick: Patch Set 2:
And I'd like to see a test. Is there a valid way that this change can matter that's not a data race? Do we not detect data races when reflect is used? Please don’t reply on this GitHub thread. Visit golang.org/cl/187577. |
Message from 填佳 姚: Patch Set 2:
I'm sorry that i forgot to close this change. This issue #33275 has been solved in https://go-review.googlesource.com/c/go/+/191197 Please don’t reply on this GitHub thread. Visit golang.org/cl/187577. |
this modification prevents the fmt panicking with reason index out of range
Fixes #33275
Change-Id: I81fbd1997b8cd1f5f30549eb4640b516105d1dcc
This PR will be imported into Gerrit with the title and first
comment (this text) used to generate the subject and body of
the Gerrit change.
Please ensure you adhere to every item in this list.
More info can be found at https://github.com/golang/go/wiki/CommitMessage
net/http: frob the quux before blarfing
"This change modifies Go to ___________"
really needed (ASCII art, table, or long link)
Fixes #1234
orUpdates #1234
(the latter if this is not a complete fix) to this comment
golang/go
you can use theowner/repo#issue_number
syntax:Fixes golang/tools#1234
Our Gerrit server & GitHub bots enforce CLA compliance instead.