proposal: cmd/vet: add a new analyzer to check for incorrect slice length initialization

[cuishuang]

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:

https://github.com/prometheus/prometheus/pull/14702#issuecomment-2302569768

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

https://github.com/uber/cadence/pull/6293

https://github.com/prometheus/prometheus/pull/15026

https://github.com/vitessio/vitess/pull/16674

https://github.com/kedacore/keda/pull/6179

https://github.com/external-secrets/external-secrets/pull/3964

https://github.com/brianvoe/gofakeit/pull/365

https://github.com/fission/fission/pull/3018

https://github.com/DataDog/datadog-agent/pull/29744

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

https://github.com/ccfos/nightingale/pull/2169

https://github.com/gookit/color/pull/97

https://github.com/vdaas/vald/pull/2672

https://github.com/supabase/auth/pull/1788

https://github.com/pufferpanel/pufferpanel/pull/1367

https://github.com/juju/juju/pull/18176

https://github.com/go-spatial/tegola/commit/0f3131fbe428368012757aecfe38f57ec1844c6a

https://github.com/lxc/incus/pull/1285

https://github.com/yunionio/cloudpods/pull/21346

https://github.com/taubyte/tau/pull/253

https://github.com/fleetdm/fleet/pull/22608

https://github.com/antrea-io/antrea/pull/6715

https://github.com/tdewolff/canvas/pull/315

https://github.com/Consensys/gnark/pull/1288

https://github.com/superfly/flyctl/pull/3982

https://github.com/bazelbuild/rules_go/pull/4133

https://github.com/zitadel/oidc/pull/658

https://github.com/jhump/protoreflect/pull/629

https://github.com/apache/rocketmq-client-go/pull/1171

https://github.com/edgexfoundry/edgex-go/pull/4938

https://github.com/dolthub/doltgresql/pull/812

https://github.com/apache/trafficcontrol/pull/8091

https://github.com/pingcap/tidb-operator/pull/5755

https://github.com/botlabs-gg/yagpdb/pull/1734

https://github.com/Altinity/clickhouse-backup/pull/1019

https://github.com/openshift/installer/pull/9072

https://github.com/GoogleCloudPlatform/magic-modules/pull/11919

https://github.com/openmeterio/openmeter/pull/1615

https://github.com/target/goalert/pull/4090

https://github.com/kubeovn/kube-ovn/pull/4579

https://github.com/syyongx/php2go/pull/49

https://github.com/fluid-cloudnative/fluid/pull/4335

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

https://github.com/kubernetes/kubernetes/pull/127785

https://github.com/apache/dubbo-go/pull/2734

https://github.com/letsencrypt/boulder/pull/7725

https://github.com/cortexproject/cortex/pull/6237

https://github.com/kubeedge/kubeedge/pull/5895

https://github.com/grafana/mimir/pull/9449

https://github.com/rocboss/paopao-ce/pull/581

https://github.com/authelia/authelia/pull/7720

https://github.com/cilium/cilium/pull/35164

https://github.com/git-lfs/git-lfs/pull/5874

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

https://github.com/cosmos/ibc-go/pull/6444

https://github.com/minio/minio/pull/19567

https://github.com/VictoriaMetrics/VictoriaMetrics/pull/6897

https://github.com/hyperledger/fabric/pull/4956

https://github.com/grafana/pyroscope/pull/3600

https://github.com/cosmos/cosmos-sdk/pull/21494#pullrequestreview-2274005038

https://github.com/anchore/grype/pull/2133

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

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

https://github.com/stashapp/stash/pull/5327

https://github.com/trufflesecurity/trufflehog/pull/3293

https://github.com/c9s/bbgo/pull/1724#issuecomment-2322959753

https://github.com/cosmos/cosmos-sdk/pull/22006

https://github.com/FerretDB/FerretDB/pull/4598

https://github.com/dagger/dagger/pull/8612

https://github.com/letsencrypt/boulder/pull/7731

https://github.com/Layr-Labs/eigenda/pull/767

https://github.com/wal-g/wal-g/pull/1800

https://github.com/VictoriaMetrics/VictoriaMetrics/pull/7161

ane more in review process.

[gabyhelp]

Related Issues and Documentation

• fmt: fix formatter init.
• runtime: fix slice init length

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

[gopherbot]

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

[gopherbot]

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

[gopherbot]

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

[timothy-king]

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.)

[zigo101]

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

[adonovan]

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, ...).

[cuishuang]

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!

[cuishuang]

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.

[timothy-king]

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 https://github.com/go-spatial/tegola/commit/0f3131fbe428368012757aecfe38f57ec1844c6a 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. As is storing it in a field has been fine in these examples. 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 be fine. 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.

[cuishuang]

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 bazel-contrib/rules_go#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. As is storing it in a field has been fine in these examples. 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 be fine. 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.

Thanks for your reply and review. I have had a lot of discussions with @adonovan about this on Gerrit.

Under adonovan's suggestion and guidance, I have now added the SuggestedFix feature.

Regarding this fix in https://github.com/akuity/kargo/pull/2648/files, it highlights the difference between using sli[i] and append, but the end result is the same. I might prefer using append, but all the way is ok.

Regarding whether to extend the existing appends analyzer or to add a new one, here are my thoughts: https://go-review.googlesource.com/c/tools/+/619915/comments/0e76d631_e6c99d92

Regarding other questions, such as "What is the 'right' way to do this? Do we need to give folks a clear escape hatch?" I'm also not sure, so perhaps I should listen to other‘s suggestions.

Overall, at least for now, adding such checks is valuable, and there are many details that can still be discussed and improved.

I will pay attention to and follow the final advice of the Go team. Thank you again!