strings: copying but not using a Builder after first use leads to runtime crash (generates heap -> stack pointer) #47276
Comments
ALTree
changed the title
ALTree
added
the
NeedsInvestigation
|
The example:
The strack trace was generated from different code (it references I'd recommend running your code with |
|
I can reproduce the crash with the snippet above on 1.16.5 if I set an higher GOMAXPROCS (like 4 or 8). I also think the crash is caused by the fact that you're copying around non-empty |
|
I'm sorry the stack trace was generated from different code. I tried to show the logic of the code. Thank you for your kind feedback. |
|
@ALTree - can you please attach the panic to this issue? What platform? I haven't been able to reproduce on Linux/amd64. The original report is for Darwin. @momotaro98 - it looks like this might be a real problem since someone has been able to reproduce a crash with the example code. This issue probably should be reopened. |
|
Cc @mknyszek |
But the example code copies around |
|
The runtime still shouldn't throw and crash. That indicates either the runtime, compiler, or @ALTree are you able to provide a failed stack trace from the example code? Which platform (or |
|
|
I think I have a hunch why this happens. The alleged bad pointer in the original post is supposed to be a builder pointing to itself (note that the GC was currently scanning So the question then is why the GC has any problems with this. I believe the answer is that The reason why this happens is the tricky As others have said, this is a valid failure mode for copying a One possibility is to make the self pointer a FTR, this is not something can be easily fixed in the runtime: we currently do not carry type information around for any object. Thus GC doesn't have access to type information, and even if it did, checking for I'm going to reopen this and update the title to indicate the UX issue here. I'll dig around too and see if I can find a duplicate. EDIT: Technically the error message is accurate in the sense of "incorrect use of unsafe." :) The |
mknyszek
changed the title
mknyszek
added
the
NeedsDecision
gopherbot
removed
the
NeedsInvestigation
|
I think fixing the UX issue here would be nice, but I'm not sure how. Open to suggestions. I couldn't find a similar issue filed anywhere. |
|
I wonder if it would be possible to write a vet check to warn against copying a "live" strings.Builder? We already warn when copying sync.Mutex as things stand. |
|
The other option is to fix #7921. Then we can remove the |
|
I think fixing #7921 is doable for Go 1.18, but I wouldn't feel comfortable backporting it to 1.16/1.17. |
|
Another option would be to only perform the That wouldn't detect memory corruption as readily in non- |
|
Yet another option might be to use type Builder struct {
buf []byte
}
func (b *Builder) copyCheck() {
i := len(buf)
if i == cap(buf) {
return // Can't check for copying if the buffer is exactly full.
}
if b := buf[:cap(buf)][i]; b != 0 {
// This builder has not written to buf[i] yet,
// so if it is nonzero it must have been written through a (disallowed) copy.
panic("strings: illegal use of non-zero Builder copied by value")
}
} |
|
Or, we could even combine the above two approaches: use an opportunistic check via |
|
I did a preliminary experiment of extending the "copylock" checker to check for the copying of "strings.Builder", e.g. Running this extension of a few open-source cloud projects doesn't generate any finding. This may indicate that its frequency is too low to be a vet checker. BTW, here are the unit tests: |
bcmills
added
the
compiler/runtime
mknyszek
changed the title
|
Adding to the 1.24 milestone because this is a pretty bad failure mode. If nothing else we could at least get "go vet" to complain about this, much as it does for copying a |
|
Given that we are in the 1.24 freeze, I think this is not appropriate to start a new vet check. Moving this to 1.25. The requirements for
vs.
This does suggest reusing the same analyzer logic. There are operations that could conceivably leave the builder in the zero-value (Len, Cap), but it is really unclear why one would call them on 'a must be zero-value'. And there is a slightly open question about copying a strings.Builder after Maybe there is a case for worrying about control-flow and 'must not be zero-ness', but I am a bit doubtful we need more than what copylocks does for sync.Mutex. I think we should just try the existing logic as a gopls check. @findleyr thoughts? |
|
I agree that it is probably sufficient to have the same semantics as copylock. Is there any use case for copying a zero builder? A reasonable step forward might be to run an ecosystem analysis on the proposed new check, following up on the analysis by @guodongli-google in #47276 (comment). I suspect that because it is a runtime panic to misuse a strings.Builder in this way, true positives of such a checker are exceedingly rare, but an analysis would also turn up any false positives due to copied zero values. I am fine with piloting this check in gopls, though I think we should only do so if it will eventually be promoted to vet. |
|
Change https://go.dev/cl/635339 mentions this issue: |
This CL fixes a number of (all true positive) findings of vet's
copylock analyzer patched to treat the Bu{ff,uild}er types
as non-copyable after first use.
This does require imposing an additional indirection
between noder.writer and Encoder since the field is
embedded by value but its constructor now returns a pointer.
Updates #25907
Updates #47276
Change-Id: I0b4d77ac12bcecadf06a91709e695365da10766c
Reviewed-on: https://go-review.googlesource.com/c/go/+/635339
Reviewed-by: Robert Findley <rfindley@google.com>
Commit-Queue: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
|
https://go.dev/cl/635339 is merged, so GOROOT is (for now) free of copying of strings.Builder and bytes.Buffer. However, I have not added a noCopy field to strings.Builder or bytes.Buffer, which would cause vet's copylocks check to report copying of values of those types, because the documented rule is that you must not copy non-zero values, which is beyond vet's ability to discriminate. |
|
Change https://go.dev/cl/674096 mentions this issue: |
Updates #26462 Updates #25907 Updates #47276 Updates #48398 Change-Id: Ic64fc8d0c284f6e5aa383a8d417fa5768dcd7925 Reviewed-on: https://go-review.googlesource.com/c/go/+/674096 Auto-Submit: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Michael Knyszek <mknyszek@google.com>
This CL fixes a number of (all true positive) findings of vet's
copylock analyzer patched to treat the Bu{ff,uild}er types
as non-copyable after first use.
This does require imposing an additional indirection
between noder.writer and Encoder since the field is
embedded by value but its constructor now returns a pointer.
Updates golang#25907
Updates golang#47276
Change-Id: I0b4d77ac12bcecadf06a91709e695365da10766c
Reviewed-on: https://go-review.googlesource.com/c/go/+/635339
Reviewed-by: Robert Findley <rfindley@google.com>
Commit-Queue: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Updates golang#26462 Updates golang#25907 Updates golang#47276 Updates golang#48398 Change-Id: Ic64fc8d0c284f6e5aa383a8d417fa5768dcd7925 Reviewed-on: https://go-review.googlesource.com/c/go/+/674096 Auto-Submit: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Michael Knyszek <mknyszek@google.com>
Uh oh!
There was an error while loading. Please reload this page.
What version of Go are you using (
go version)?Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env)?go envOutputWhat did you do?
https://play.golang.org/p/RRI3-srzVrR
What did you expect to see?
I don't see any error message
What did you see instead?
The text was updated successfully, but these errors were encountered: