proposal: x/tools/go/analysis/passes/appends: check for incorrect slice length initialization #69872
Comments
|
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: |
|
Change https://go.dev/cl/619935 mentions this issue: |
|
Change https://go.dev/cl/618875 mentions this issue: |
|
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 |
|
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 |
ianlancetaylor
changed the title
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! |
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. |
|
Change https://go.dev/cl/620316 mentions this issue: |
To answer my own question, the escape hatch is to write It seems okay to encourage this as an escape hatch as the checker is trying to find cases where slice's |
|
After communicating with the two maintainers and receiving very good suggestions, we decided not to add a new analyzer, but to add these features based on the previous appends analyzer. Does this still need to be proposed? |
cuishuang
changed the title
|
Yes, major feature changes to analyzers included in vet do need proposals. |
Thanks. I have moved the code to the |
|
Thanks Alan. Those FPs in the first 10 or so examples are clear cases of wanting to generate |
Thanks @adonovan ! In my practice, the main false positives also come from 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 |
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. |
|
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 |
|
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? |
(And also randomly selected 20 from the remaining 775.) The detailed result: https://github.com/cuishuang/appends-analysis out := make([]rune, len(slice))
out = out[:0]
In summary, when the slice type is 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%. |
seankhliao
changed the title
|
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. |
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. |
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]
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
superseriousbusiness/gotosocial#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/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.
The text was updated successfully, but these errors were encountered: