proposal: x/tools/go/analysis/passes/appends: check for incorrect slice length initialization

Proposal Details

The following code exists in many projects, and developers actually want [0 1 2], but due to the initialization error of slice, the final result is [0 0 0 0 1 2]

package main

import "fmt"

func main() {

	sli := make([]int, 3)

	for i := range 3 {
		sli = append(sli, i)
	}

	fmt.Println(sli) // the result is [0 0 0 0 1 2]

}

The online demo: https://go.dev/play/p/q1BcVCmvidW

Over the past few months, I have conducted extensive research and analysis, and also submitted pull requests to fix issues in many well-known Go projects such as prometheus, zap, vitess. Below are some pull requests submitted by me and others related to this problem.

Due to limitations in search skills and time, I only checked records of such issues in the past few months. The history of more such issues has not been traced back. But I think it's already enough

I would like to propose adding a new analyzer to go vet that can detect such situations, thereby avoiding these issues in the future.

Now I have already completed an initial version of the code, and if the proposal is approved, I would be happy to refine it and add the necessary test cases.

The merged pr:

prometheus/prometheus#14702 (comment)

uber-go/zap#1461

cadence-workflow/cadence#6293

prometheus/prometheus#15026

vitessio/vitess#16674

kedacore/keda#6179

external-secrets/external-secrets#3964

brianvoe/gofakeit#365

fission/fission#3018

DataDog/datadog-agent#29744

https://github.com/superseriousbusiness/gotosocial/pull/3382

ccfos/nightingale#2169

gookit/color#97

vdaas/vald#2672

supabase/auth#1788

pufferpanel/pufferpanel#1367

juju/juju#18176

go-spatial/tegola@0f3131f

lxc/incus#1285

yunionio/cloudpods#21346

taubyte/tau#253

fleetdm/fleet#22608

antrea-io/antrea#6715

tdewolff/canvas#315

Consensys-Incorporated/gnark#1288

superfly/flyctl#3982

bazel-contrib/rules_go#4133

zitadel/oidc#658

jhump/protoreflect#629

apache/rocketmq-client-go#1171

edgexfoundry/edgex-go#4938

dolthub/doltgresql#812

apache/trafficcontrol#8091

pingcap/tidb-operator#5755

botlabs-gg/yagpdb#1734

Altinity/clickhouse-backup#1019

openshift/installer#9072

GoogleCloudPlatform/magic-modules#11919

openmeterio/openmeter#1615

target/goalert#4090

kubeovn/kube-ovn#4579

syyongx/php2go#49

fluid-cloudnative/fluid#4335

akuity/kargo#2648

kubernetes/kubernetes#127785

apache/dubbo-go#2734

letsencrypt/boulder#7725

cortexproject/cortex#6237

kubeedge/kubeedge#5895

grafana/mimir#9449

rocboss/paopao-ce#581

authelia/authelia#7720

cilium/cilium#35164

git-lfs/git-lfs#5874

https://github.com/hashicorp/nomad/pull/24109/files

cosmos/ibc-go#6444

minio/minio#19567

VictoriaMetrics/VictoriaMetrics#6897

hyperledger/fabric#4956

grafana/pyroscope#3600

cosmos/cosmos-sdk#21494 (review)

anchore/grype#2133

https://github.com/ethereum-optimism/optimism/pull/11542/files

https://github.com/libp2p/go-libp2p/pull/2938/files

stashapp/stash#5327

trufflesecurity/trufflehog#3293

c9s/bbgo#1724 (comment)

cosmos/cosmos-sdk#22006

FerretDB/FerretDB#4598

dagger/dagger#8612

letsencrypt/boulder#7731

Layr-Labs/eigenda#767

wal-g/wal-g#1800

VictoriaMetrics/VictoriaMetrics#7161

harmony-one/harmony#4767

stackrox/stackrox#13028

stefanprodan/timoni#430

Altinity/clickhouse-operator#1523

iotexproject/iotex-core#4412

ane more in review process.

Related Issues and Documentation

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

Change https://go.dev/cl/619915 mentions this issue: go/analysis: add a new analyzer to check for incorrect slice length initialization

Change https://go.dev/cl/619935 mentions this issue: cmd: add a new analyzer to check for incorrect slice length initialization

Change https://go.dev/cl/618875 mentions this issue: runtime: fix slice init length

What is the proposed criteria to decide when vet should issue a report? It is hard to decide whether to accept a vet proposal without this being specified. We need to assess the likely false positive and negatives rates of the criteria. Thanks.

(You can try to turn the included CL into natural language.)

I'm surprised that there are so many misuses of the append function.

Guessing from logic of the CL I just reviewed, the criterion is:

Vet should report a diagnostic when appending to a slice s if there exists an assignment s = make(T, n) where n is not a constant zero and there are no expressions of the form s[i] or copy(s, ...).

What is the proposed criteria to decide when vet should issue a report? It is hard to decide whether to accept a vet proposal without this being specified. We need to assess the likely false positive and negatives rates of the criteria. Thanks.

(You can try to turn the included CL into natural language.)

Yes, this is the issue I initially wanted to discuss on the CL page. Thanks to @adonovan for the detailed summary.

Additionally, through extensive practice, I have encountered several challenges and gained some insights: How do we balance false positives and omissions? Should we aim to comprehensively identify similar issues and accept a high rate of false positives, or should we focus on precision, which might lead to some omissions? This balance certainly warrants further discussion.

Specifically regarding the code, I’ve noticed that when slice are processed with binary.LittleEndian.PutUint16, they often lead to false positives. Moreover, if a slice is initialized in function A but only appended to after several layers of calls in function Z, should we address this situation? Or should we only consider initialization and appending that occur within the same method?

Last year, with your guidance, I added the "appends" analyzer to go vet, so I have some experience in this area.

However, it’s clear that this situation is more complex than last year, and there are more details to discuss. I look forward to your suggestions.

Thank you all!

I'm surprised that there are so many misuses of the append function.

Yes, if using sli[i] for assignment, there is no such problem, but many people like me are used to using append. So I think it can also be considered as an initialization issue with the slice.

Based on my limited investigation and analysis of high star libraries related to the Go ecosystem, it seems worthwhile to add such an analyzer.

I took a look at several of these reports. Most looks good. Some things to discuss. Here are my notes.

These all look like good reports to me. I think we can have stricter criteria and still report these. They are very similar.

https://github.com/prometheus/prometheus/pull/14702/files
https://github.com/uber/cadence/pull/6293/files
https://github.com/vitessio/vitess/pull/16674/files
https://github.com/kedacore/keda/pull/6179/files
https://github.com/brianvoe/gofakeit/pull/365/files
https://github.com/fission/fission/pull/3018/files
https://github.com/DataDog/datadog-agent/pull/29744/files
https://github.com/superseriousbusiness/gotosocial/pull/3382/files
https://github.com/ccfos/nightingale/pull/2169/files
https://github.com/gookit/color/pull/97/files
https://github.com/supabase/auth/pull/1788/files
https://github.com/pufferpanel/pufferpanel/pull/1367/files
https://github.com/juju/juju/pull/18176/files
go-spatial/tegola@0f3131f
https://github.com/lxc/incus/pull/1285/files
https://github.com/yunionio/cloudpods/pull/21346/files
https://github.com/taubyte/tau/pull/253/files
https://github.com/fleetdm/fleet/pull/22608/files
https://github.com/antrea-io/antrea/pull/6715/files
https://github.com/tdewolff/canvas/pull/315/files
https://github.com/Consensys/gnark/pull/1288/files
https://github.com/superfly/flyctl/pull/3982/files
https://github.com/bazel-contrib/rules_go/pull/4133
https://github.com/zitadel/oidc/pull/658/files
https://github.com/jhump/protoreflect/pull/629/files
https://github.com/apache/rocketmq-client-go/pull/1171/files
https://github.com/edgexfoundry/edgex-go/pull/4938/files
https://github.com/apache/trafficcontrol/pull/8091/files
https://github.com/pingcap/tidb-operator/pull/5755/files
https://github.com/botlabs-gg/yagpdb/pull/1734/files
https://github.com/Altinity/clickhouse-backup/pull/1019/files
https://github.com/openshift/installer/pull/9072/files
https://github.com/cortexproject/cortex/pull/6237/files
https://github.com/letsencrypt/boulder/pull/7725/files
https://github.com/kubernetes/kubernetes/pull/127785/files
https://github.com/fluid-cloudnative/fluid/pull/4335/files
https://github.com/syyongx/php2go/pull/49/files
https://github.com/kubeovn/kube-ovn/pull/4579/files
https://github.com/target/goalert/pull/4090/files
https://github.com/openmeterio/openmeter/pull/1615/files
https://github.com/openmeterio/openmeter/pull/1615/files
https://github.com/cuishuang/vald/blob/2bb30dac79f0d9afdb0f809f0ca5b8eef0e62d75/internal/net/grpc/client.go
https://github.com/GoogleCloudPlatform/magic-modules/pull/11919/files

This also looks like a good report, but had a more interesting fix:

https://github.com/akuity/kargo/pull/2648/files

These I am not as sure about. It would not be terrible to report these, but it would also not be too bad to not report them. Some of these should be rewritten as literals. Could we suggest alternative rewrites from the Analyzer?

https://github.com/dolthub/doltgresql/pull/812/files
https://github.com/apache/dubbo-go/pull/2734/files

The proposed criteria would only have reported TestAzureKeyVaultSecretManagerGetAllSecrets in:

https://github.com/external-secrets/external-secrets/pull/3964/files
This is a good example for why we need to exclude 0.

We should probably not report either of these from vet. This suggests to me we need to exclude slice expressions too.

https://github.com/prometheus/prometheus/pull/15026/files
https://github.com/uber-go/zap/pull/1461/files

I am still not exactly sure I have a clear theory of what we are reporting yet. But the heuristics seem to be hitting on something promising. Indexing and slicing is out. range is fine. len is fine. Passing the slice as an argument or a variadic expansion... is also fine. Storing it in a field has been fine in these examples so far. I am not sure what this adds up to.

Another point, it would be good for us to have negative examples too. Making an all zero value slice needs to not be reported. And we should have guidance on how to create {0,0,0,1,2,3}. The proposal is going to flag a correct way to create this value. What is the 'right' way to do this? Do we need to give folks a clear escape hatch?

FWIW I think the proposal should be phrased 'as what to report' rather than 'add a new analyzer'. This could plausibly go into the "appends" analyzer.

Yes, major feature changes to analyzers included in vet do need proposals.

Thanks. I have moved the code to the appends directory. Some comments may need to be optimized. Please review it when you have time.

I ran the new analyzer (at patch set 18 of CL 619915) over about 15,000 modules, and got 1225 new findings. Here are a random 50.

Inspecting the first 10, at least 7 are false positives, most relating to functions like binary.BigEndian.PutUint32 that fill a fixed-size buffer that is later appended. It does turn up some real bugs though. More work is needed to reduce the false-positive rate beneath our target of 5%.

Thanks Alan. Those FPs in the first 10 or so examples are clear cases of wanting to generate {0,0,0,0,1,2,3,4}. We will need a more specific criteria.

@adonovan

Additionally, through extensive practice, I have encountered several challenges and gained some insights: How do we balance false positives and omissions? Should we aim to comprehensively identify similar issues and accept a high rate of false positives, or should we focus on precision, which might lead to some omissions? This balance certainly warrants further discussion.

Specifically regarding the code, I’ve noticed that when slice are processed with binary.LittleEndian.PutUint16, they often lead to false positives. Moreover, if a slice is initialized in function A but only appended to after several layers of calls in function Z, should we address this situation? Or should we only consider initialization and appending that occur within the same method?

Thanks @adonovan ! In my practice, the main false positives also come from binary.LittleEndian.PutUint16 related methods.

I agree with what @timothy-king said, maybe we need to add some more standards on the basis of the above three standards to reduce the false positive rate. ( But I am surprised why there are so many cases where the original intention is to get {0,0,0,0,1,2,3,4} )

Do the two maintainers and community developers have any suggestions?

My current idea is that if sli is called by binary.LittleEndian.PutUint16 related methods, it is ignored, just like it has been called by copy or sli[i]

My current idea is that if sli is called by binary.LittleEndian.PutUint16 related methods, it is ignored, just like it has been called by copy or sli[i]

We can try to list all of the stdlib functions that might be used to intentionally append to a slice, but my intuition is that this will not be sufficient. We have to also consider user appending functions.

My recommendation is to go back to the original examples and to try to tease out why we think those examples are bugs more precisely. We can re-implement once we better understand this.

We can try to list all of the stdlib functions that might be used to intentionally append to a slice, but my intuition is that this will not be sufficient. We have to also consider user appending functions.

My recommendation is to go back to the original examples and to try to tease out why we think those examples are bugs more precisely. We can re-implement once we better understand this.

Okay, I will list out the methods in the standard library that trigger false positives, such as binary.LittleEndian.PutUint16. Regarding the "original examples," I want to confirm whether you are referring to the initial PRs I listed that were merged, or the results provided by Alan. I believe it should be the latter. We just need to analyze the common situations that lead to false positives, and by modifying the code accordingly, we can significantly reduce the false positive rate to an acceptable level.

Most of the false positives are caused by slices of type []byte. Ignoring checks for this type of slice could immediately reduce the false positive rate, but this approach seems a bit aggressive.

At present, through the analysis of the corpus information provided by Alan and my previous analysis of the go project on GitHub, the following situations occur when calling a method of the standard library and a false positive occurs:

        binary.LittleEndian.PutUint16(): https://github.com/cloudflare/circl/blob/main/abe/cpabe/tkn20/internal/tkn/policy.go#L84:8
	binary.LittleEndian.PutUint32(): https://github.com/nknorg/nkn/blob/master/chain/store/store.go#L480:12
	binary.LittleEndian.PutUint64(): https://github.com/ethersphere/bee/blob/master/pkg/storage/storagetest/storage.go#L101:8   

	binary.LittleEndian.Uint16()
	binary.LittleEndian.Uint32()
	binary.LittleEndian.Uint64()

	binary.LittleEndian.AppendUint16()
	binary.LittleEndian.AppendUint32()
	binary.LittleEndian.AppendUint64()

	binary.BigEndian.PutUint16():  https://github.com/XZB-1248/Spark/blob/master/client/service/desktop/desktop.go#L233:8
	binary.BigEndian.PutUint32()
	binary.BigEndian.PutUint64():  https://github.com/bnb-chain/bsc/blob/master/core/vm/contracts_lightclient.go#L105:11

	binary.BigEndian.Uint16()
	binary.BigEndian.Uint32()
	binary.BigEndian.Uint64()

	binary.BigEndian.AppendUint16()
	binary.BigEndian.AppendUint32()
	binary.BigEndian.AppendUint64()

I have submitted a new patch to exclude the above situation.

But if this method is not in the standard library but is user-defined, should it still be considered?

(Perhaps based on practical experience, the false positive rate after excluding binary.BigEndian is acceptable)

Are you able to estimate the the false positive rate when these cases are excluded? The file attached above contains the complete list of 1225 findings. Can you sample a couple of dozen?

Are you able to estimate the the false positive rate when these cases are excluded? The file attached above contains the complete list of 1225 findings. Can you sample a couple of dozen?

I conducted a detailed analysis, and out of 1,225 cases, approximately 375 of them are related to the usage of the standard library's methods involving binary.BigEndian or binary.LittleEndian to process slices. (I simply wrote a program to detect whether the page contained the keywords binary.BigEndian or binary.LittleEndian. I didn’t further check if slices were actually passed as parameters to these methods, but the margin of error is very small. )

For the remaining 850 cases, I analyzed the first 100.

(And also randomly selected 20 from the remaining 775.)

The detailed result: https://github.com/cuishuang/appends-analysis

Consider the first 100 as an example: 24 of them were clearly false positives. Among these, 22 were due to calls to the standard library functions rand.Read(sli) (cases 6, 7, 8, 9, 10, 11, 12, 15, 27, 29, 30, 31, 35, 36, 66), io.ReadFull(rand.Reader, sli) (cases 2, 18, 19, 38), cipher.XORKeyStream (cases 77, 78), and binary.PutUvarint (case 86), which we can easily avoid reporting
.

In case 87, the usage of unicode.ToUpper does not seem to be a false positive in my opinion.

Additionally:

In case 4:

out := make([]rune, len(slice))
out = out[:0]

This is essentially equivalent to out := make([]rune, 0, len(slice)). Perhaps it is necessary to exclude operations like sli[i:j].

For cases 5, 62, and 63, there are copy operations, but we still flagged these for detection. We need to investigate why they were not ignored.

There are about a dozen cases still in the "todo" state, mainly because they involve calls to user-defined methods, such as:

Case 1: runes.Convert from "9fans.net/go/cmd/acme/internal/runes"
Case 16: util.ReadAtMost
Case 60: fee.Amount.BigInt().FillBytes(sli)
Cases 79, 80: Wrap
Cases 93, 97: utils.NewBitWriter(utils.NewWriterAtBuffer(sli)

Some of these might not be false positives, It's actually a bug that needs to be fixed. such as:
Cases 48, 53, 55

Aside from these, the rest are fairly standard errors and should be reported.

In summary, when the slice type is []byte, and it's often used in cryptographic contexts, the use of make([]byte, n) is usually exactly what the developer intended.

If we handle the listed methods from the standard library while ignoring user-defined methods, I believe the false positive rate can be reduced to around 5%.

I have put this proposal on Hold until we have a promising prototype implementation.

I have put this proposal on Hold until we have a promising prototype implementation.

OK, thanks. I'll keep an eye on its progress.

If we reach an agreement, I'll be happy to implement its code.

If we reach an agreement, I'll be happy to implement its code.

The order is backwards: what this proposal needs to move forwards is a new idea, a prototype, an experiment, and some encouraging data; agreement will follow from that.

If we reach an agreement, I'll be happy to implement its code.

The order is backwards: what this proposal needs to move forwards is a new idea, a prototype, an experiment, and some encouraging data; agreement will follow from that.

OK, I understand.

My current idea is to add checks for special methods in the standard library (such as binary.BigEndian), or to exclude the []byte type more aggressively.

This is indeed a widespread problem, but it is not easy to reduce the false positive rate in implementation. Maybe there will be a better solution later.

Thanks for your patience guidance and review.

Navigation Menu

Repository navigation

proposal: x/tools/go/analysis/passes/appends: check for incorrect slice length initialization #69872

Proposal Details

6 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

proposal: x/tools/go/analysis/passes/appends: check for incorrect slice length initialization #69872

Description

Proposal Details

Activity

gabyhelp commented on Oct 14, 2024

gopherbot commented on Oct 14, 2024

gopherbot commented on Oct 14, 2024

gopherbot commented on Oct 14, 2024

timothy-king commented on Oct 14, 2024

zigo101 commented on Oct 14, 2024

adonovan commented on Oct 14, 2024

cuishuang commented on Oct 15, 2024

cuishuang commented on Oct 15, 2024

timothy-king commented on Oct 17, 2024

6 remaining items

cuishuang commented on Oct 23, 2024

adonovan commented on Oct 23, 2024

timothy-king commented on Oct 24, 2024

cuishuang commented on Oct 24, 2024

timothy-king commented on Oct 24, 2024

cuishuang commented on Oct 24, 2024

cuishuang commented on Oct 24, 2024

adonovan commented on Oct 24, 2024

cuishuang commented on Oct 25, 2024

adonovan commented on Nov 20, 2024

cuishuang commented on Nov 21, 2024

adonovan commented on Nov 21, 2024

cuishuang commented on Nov 21, 2024

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions