bytes: TrimXXX makes it easy to unintentionally overwrite slice #43341

shmsr · 2020-12-23T09:00:25Z

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

$ go version

go version go1.15.5 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (`go env`)?

go env Output

$ go env
GO111MODULE=""

GOARCH="amd64"

GOBIN=""

GOCACHE="/Users/subhamsarkar/Library/Caches/go-build"

GOENV="/Users/subhamsarkar/Library/Application Support/go/env"

GOEXE=""

GOFLAGS=""

GOHOSTARCH="amd64"

GOHOSTOS="darwin"

GOINSECURE=""

GOMODCACHE="/Users/subhamsarkar/go/pkg/mod"

GONOPROXY=""

GONOSUMDB=""

GOOS="darwin"

GOPATH="/Users/subhamsarkar/go"

GOPRIVATE=""

GOPROXY="https://proxy.golang.org,direct"

GOROOT="/usr/local/Cellar/go/1.15.5/libexec"

GOSUMDB="sum.golang.org"

GOTMPDIR=""

GOTOOLDIR="/usr/local/Cellar/go/1.15.5/libexec/pkg/tool/darwin_amd64"

GCCGO="gccgo"

AR="ar"

CC="clang"

CXX="clang++"

CGO_ENABLED="1"

GOMOD=""

CGO_CFLAGS="-g -O2"

CGO_CPPFLAGS=""

CGO_CXXFLAGS="-g -O2"

CGO_FFLAGS="-g -O2"

CGO_LDFLAGS="-g -O2"

PKG_CONFIG="pkg-config"

GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/n2/w00w99g93cj26xl42msl6lc80000gp/T/go-build468451499=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Trim and friends (TrimXXX) in the bytes package seem to unintentionally overwrite slices. Well, this could be working as designed but do refer:

So, based on CLs that fixed the issues listed above, I think it'd be appropriate to match the capacity to the length so that it could be avoided and match the behaviour of other functions.

package main

import (
	"bytes"
	"fmt"
)

// func FixTrimSuffix(s, suffix []byte) []byte {
// 	if bytes.HasSuffix(s, suffix) {
// 		// Match capacity and length. Fixed bytes.TrimSuffix
// 		return s[: len(s)-len(suffix) : len(s)-len(suffix)]
// 	}
// 	return s
// }

func main() {
	b := []byte{'h', 'e', 'l', 'l', 'o'}
	t := []byte{'l', 'o'}

	fmt.Printf("b: %s\nt: %s\n", b, t)

	fmt.Printf("\n~ TrimSuffix t from b\n")
	bytesTrim := bytes.TrimSuffix(b, t)
	fmt.Printf("b: %s\nbytesTrim: %s\n", b, bytesTrim)

	fmt.Printf("\n~ Append xy to bytesTrim\n")
	bytesTrim = append(bytesTrim, 'x', 'y')
	fmt.Printf("b: %s\nbytesTrim: %s\n", b, bytesTrim)
}

Link to the playground: https://play.golang.org/p/yGJxI_Jnkk6

What did you expect to see?

b: hello
t: lo

~ TrimSuffix t from b
b: hello
bytesTrim: hel

~ Append xy to bytesTrim
b: hello
bytesTrim: helxy

What did you see instead?

b: hello
t: lo

~ TrimSuffix t from b
b: hello
bytesTrim: hel

~ Append xy to bytesTrim
b: helxy
bytesTrim: helxy

The text was updated successfully, but these errors were encountered:

shmsr · 2020-12-23T09:01:11Z

If this issue needs to be fixed then I'd like to send a CL for this if it's fine.

shmsr · 2020-12-23T11:57:44Z

@seankhliao I know how slices work. Look at CL for #30169!

seankhliao · 2020-12-23T13:53:38Z

I see, after reading the linked issues, 👍

ianlancetaylor · 2020-12-23T16:51:25Z

It would help to see the kind of analysis that was done in #21149 (comment). My intuition is that TrimSuffix in particular is a case where people might want to write things like b = append(bytes.TrimSuffix(b, ".go"), ".o"...) in which case forcing a new allocation would be undesirable.

shmsr · 2020-12-23T17:03:45Z

Yeah, right. Okay, I'll do some research on this!

mvdan · 2020-12-25T10:44:37Z

Just to give an extra data point - I've used Trim funcs many times with the knowledge that they do not allocate. Changing that would be surprising to me, and would make perfectly valid code suddenly allocate more. That could very well mean regressions if the call is in performance-sensitive code.

go101 · 2020-12-25T14:33:15Z

If the syntax s[::] is supported, this can be easily written as

bytesTrim = append(bytesTrim[::], 'x', 'y')

b = append(bytes.TrimSuffix(b, ".go")[::], ".o"...)

to always make an allocation.

shmsr · 2021-01-16T17:37:35Z

Sorry for keep you guys waiting. So I'm skimmed through a lot of projects which were using popular packages (mostly vendored) and I think it should be safe to do the change that I was suggesting earlier. Although there's some disagreement and that's totally valid. So here I'm attaching some links where you can see the usage of bytes's Trim and friends.

In my opinion, the performance aspect of limiting the capacity might affect a small number of projects but most of the projects shouldn't see any difference. Also, it'd match the behaviour done in #21149 #30169

Here are some links:

vendor: (github.com/go-git/go-git/v5/plumbing/object/commit.go)
https://github.com/GoogleContainerTools/kaniko/blob/master/vendor/github.com/go-git/go-git/v5/plumbing/object/commit.go#L198

vendor: (github.com/go-git/go-git/v5/plumbing/object/commit.go)
https://github.com/GoogleContainerTools/kaniko/blob/ece215c18113020f9151fb25e69fc4ecc157c395/vendor/github.com/go-git/go-git/v5/plumbing/object/commit.go#L207

vendor: (github.com/moby/buildkit/frontend/dockerfile/parser/parser.go)
https://github.com/GoogleContainerTools/kaniko/blob/master/vendor/github.com/moby/buildkit/frontend/dockerfile/parser/parser.go#L228

vendor: (golang.org/x/crypto/openpgp/armor/armor.go)
https://github.com/GoogleContainerTools/skaffold/blob/master/hack/tools/vendor/golang.org/x/crypto/openpgp/armor/armor.go#L175

vendor: (golang.org/x/crypto/openpgp/armor/armor.go)
https://github.com/GoogleContainerTools/skaffold/blob/master/hack/tools/vendor/golang.org/x/crypto/openpgp/armor/armor.go#L199

https://github.com/GoogleContainerTools/skaffold/blob/master/pkg/skaffold/build/tag/git_commit.go#L166

https://github.com/Jeffail/benthos/blob/master/lib/processor/text.go#L130

vendor: (golang.org/x/crypto/ssh/keys.go)
https://github.com/PursuanceProject/pursuance/blob/develop/vendor/golang.org/x/crypto/ssh/keys.go#L67

vendor: (golang.org/x/crypto/ssh/keys.go)
https://github.com/PursuanceProject/pursuance/blob/develop/vendor/golang.org/x/crypto/ssh/keys.go#L85

vendor: (github.com/russross/blackfriday/v2/block.go)
https://github.com/docker/cli/blob/master/vendor/github.com/russross/blackfriday/v2/block.go#L310

https://github.com/jcmvbkbc/gcc-xtensa/blob/call0-4.8.2/libgo/go/encoding/pem/pem.go#L48

Do let me know if you need more examples.

dsnet · 2021-07-28T00:32:00Z

I'm not sure if isolated examples are helpful (it's fairly easy to find examples that support either semantic). What would be needed is a thorough analysis that answers something like:

Of all the usages of bytes.TrimXXX on the module proxy, what percentage provably do not depend the extra capacity?

Currently, based on the 👍 on comments above, it seems that people generally prefer the current semantic.

This comment has been minimized.

Sign in to view

go101 mentioned this issue Dec 30, 2020

proposal: spec: allow append(string, ...) and view the string argument as a byte slice #43429

Closed

dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 30, 2020

dmitshur added this to the Backlog milestone Dec 30, 2020

go101 mentioned this issue Jul 27, 2021

bytes: document capacity expectations of Split and Fields #47414

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bytes: TrimXXX makes it easy to unintentionally overwrite slice #43341

bytes: TrimXXX makes it easy to unintentionally overwrite slice #43341

shmsr commented Dec 23, 2020

shmsr commented Dec 23, 2020 •

edited

This comment has been minimized.

shmsr commented Dec 23, 2020

seankhliao commented Dec 23, 2020

ianlancetaylor commented Dec 23, 2020

shmsr commented Dec 23, 2020

mvdan commented Dec 25, 2020

go101 commented Dec 25, 2020 •

edited

shmsr commented Jan 16, 2021

dsnet commented Jul 28, 2021

bytes: TrimXXX makes it easy to unintentionally overwrite slice #43341

bytes: TrimXXX makes it easy to unintentionally overwrite slice #43341

Comments

shmsr commented Dec 23, 2020

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

Does this issue reproduce with the latest release?

What operating system and processor architecture are you using (go env)?

What did you do?

What did you expect to see?

What did you see instead?

shmsr commented Dec 23, 2020 • edited

This comment has been minimized.

shmsr commented Dec 23, 2020

seankhliao commented Dec 23, 2020

ianlancetaylor commented Dec 23, 2020

shmsr commented Dec 23, 2020

mvdan commented Dec 25, 2020

go101 commented Dec 25, 2020 • edited

shmsr commented Jan 16, 2021

dsnet commented Jul 28, 2021

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

What operating system and processor architecture are you using (`go env`)?

shmsr commented Dec 23, 2020 •

edited

go101 commented Dec 25, 2020 •

edited