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

bytes: Buffer should add copy check #48398

Closed
xpunch opened this issue Sep 15, 2021 · 8 comments
Closed

bytes: Buffer should add copy check #48398

xpunch opened this issue Sep 15, 2021 · 8 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@xpunch
Copy link

xpunch commented Sep 15, 2021

What version of Go are you using (go version)?

$ go version
go version go1.17 windows/amd64

What did you do?

package main

import (
	"bytes"
	"fmt"
)

func main() {
	var a bytes.Buffer
	a.WriteString("aaaaaaaaaaaaa-")
	b := a
	a.WriteString("a")
	fmt.Printf("Before - %s\n", a.String())
	b.WriteString("b")
	fmt.Printf("After  - %s\n", a.String())
}
Before - aaaaaaaaaaaaa-a
After  - aaaaaaaaaaaaa-b

What did you expect to see?

Provide copy check to prevent value copy of bytes.Buffer.

What did you see instead?

a influence by b

@seankhliao
Copy link
Member

Duplicate of #25907

@seankhliao seankhliao marked this as a duplicate of #25907 Sep 15, 2021
@xpunch
Copy link
Author

xpunch commented Sep 16, 2021

This is not duplicate of #25907, strings.Builder do dynamic copy check, but bytes.Buffer is leak of dynamic copy check.

@ianlancetaylor
Copy link
Contributor

I agree that this is not quite a dup of #25907. But I don't think we should add a dynamic copy check here. For strings.Builder the dynamic copy check is required for correctness, as otherwise a string value could change while the program is running. The same consideration does not arise for bytes.Buffer. It is OK to copy a bytes.Buffer, even if the results can be unexpected in some cases. It's not different than using assignment to make multiple copies of a slice.

@xpunch
Copy link
Author

xpunch commented Sep 17, 2021

Not everyone who using golang knows the internal implementation of bytes.Buffer, so we should not think the "unexpected result" is common sense. If this is not a duplicate of #25907, I think we should reopen this issue, instead of closing it roughly.

@ianlancetaylor
Copy link
Contributor

OK, I will reopen it, but I expect the result will be to close it gently. Copying a bytes.Buffer is not wrong. What is wrong is copying a bytes.Buffer and then writing to both copies. If we add a copy check to bytes.Buffer today we will most likely break existing working code.

@ianlancetaylor ianlancetaylor changed the title should add copy check for bytes.Buffer bytes.Buffer: should add copy check Sep 17, 2021
@cagedmantis cagedmantis added this to the Backlog milestone Sep 17, 2021
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 17, 2021
@xpunch
Copy link
Author

xpunch commented Sep 18, 2021

bytes.Buffer already existed for 12 years, add a copy check will break some working codes indeed.
At least we should add some comments or print some warnings in go vet or staticcheck.

// A Buffer is a variable-sized buffer of bytes with Read and Write methods.
// The zero value for Buffer is an empty buffer ready to use.
// Do not copy a non-zero Buffer.
type Buffer struct {
}

@adonovan
Copy link
Member

Not everyone who using golang knows the internal implementation of bytes.Buffer, so we should not think the "unexpected result" is common sense.

I would argue that we should. You don't need to know Buffer's internals to know that in Go, it is generally a bad idea to copy a value of type T that has methods associated with *T, because method calls may lead to subtle aliasing relationships. For example, a call may cause one field of a struct to refer to another field. Go's direct copy semantics are not appropriate for such values, because they cause both the copy and the original to refer to the original. You need to call a Clone method, if the type provides one, or avoid copying altogether.

@xpunch
Copy link
Author

xpunch commented Dec 30, 2021

We all know the consequence of copy a non-zero value of bytes.Buffer, but there is no warning or mechanism to prevent it. We don't want golang users need to know those tricks before writing their codes. Your example is correct, but bytes.Buffer is in standard library, it should be out-of-the-box. I think that's the reason why strings.Builder have dynamic copy check.

@seankhliao seankhliao changed the title bytes.Buffer: should add copy check bytes: Buffer should add copy check Dec 30, 2021
@xpunch xpunch closed this as completed Jan 15, 2023
@golang golang locked and limited conversation to collaborators Jan 15, 2024
@golang golang unlocked this conversation Mar 6, 2024
@golang golang locked as resolved and limited conversation to collaborators Mar 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants