From 052a3c8ef3c88d6c32c987258232ba15414055e0 Mon Sep 17 00:00:00 2001 From: Alexey Surikov Date: Sun, 9 Feb 2020 05:01:06 +0100 Subject: [PATCH 1/6] staticcheck: flag dubious bit shifting of fixed size integers --- staticcheck/analysis.go | 4 + staticcheck/doc.go | 23 +++++ staticcheck/lint.go | 57 ++++++++++++ staticcheck/lint_test.go | 1 + .../CheckStaticBitShift.go | 87 +++++++++++++++++++ 5 files changed, 172 insertions(+) create mode 100644 staticcheck/testdata/src/CheckStaticBitShift/CheckStaticBitShift.go diff --git a/staticcheck/analysis.go b/staticcheck/analysis.go index 75df1e120..354734494 100644 --- a/staticcheck/analysis.go +++ b/staticcheck/analysis.go @@ -259,6 +259,10 @@ var Analyzers = lintutil.InitializeAnalyzers(Docs, map[string]*analysis.Analyzer }, // Filtering generated code because it may include empty structs generated from data models. "SA9005": makeCallCheckerAnalyzer(checkNoopMarshal, facts.Generated), + "SA9006": { + Run: CheckStaticBitShift, + Requires: []*analysis.Analyzer{inspect.Analyzer}, + }, "SA4022": { Run: CheckAddressIsNil, diff --git a/staticcheck/doc.go b/staticcheck/doc.go index 319ea2707..47dcd6de2 100644 --- a/staticcheck/doc.go +++ b/staticcheck/doc.go @@ -839,4 +839,27 @@ marshaling behavior, e.g. via MarshalJSON methods. It will also not flag empty structs.`, Since: "2019.2", }, + + "SA9006": { + Title: `Dubious bit shifting of a fixed size integer value`, + Text: `Bit shifting an value past its size will always clear the value. + +For instance: + + v := int8(42) + v >>= 8 + +will always result in 0. + +This check flags bit shifiting operations on fixed size integer values only. +That is, int, uint and uintptr are never flagged to avoid potential false +positives in somewhat exotic but valid bit twiddling tricks: + + // Clear any value above 32 bits if integers are more than 32 bits. + func f(i int) int { + v := i >> 32 + v = v << 32 + return i-v + }`, + }, } diff --git a/staticcheck/lint.go b/staticcheck/lint.go index 2ccfb9e6f..a1bc5b1ab 100644 --- a/staticcheck/lint.go +++ b/staticcheck/lint.go @@ -12,6 +12,7 @@ import ( "reflect" "regexp" "regexp/syntax" + "runtime" "sort" "strconv" "strings" @@ -24,6 +25,7 @@ import ( "honnef.co/go/tools/edit" "honnef.co/go/tools/facts" "honnef.co/go/tools/functions" + "honnef.co/go/tools/gcsizes" "honnef.co/go/tools/internal/passes/buildir" "honnef.co/go/tools/internal/sharedcheck" "honnef.co/go/tools/ir" @@ -3817,3 +3819,58 @@ func CheckAddressIsNil(pass *analysis.Pass) (interface{}, error) { code.Preorder(pass, fn, (*ast.BinaryExpr)(nil)) return nil, nil } + +var ( + checkFixedLengthTypeShiftQ = pattern.MustParse(` + (Or + (AssignStmt _ (Or ">>=" "<<=") _) + (BinaryExpr _ (Or ">>" "<<") _)) + `) + wordSize = gcsizes.ForArch(runtime.GOARCH).WordSize +) + +func CheckStaticBitShift(pass *analysis.Pass) (interface{}, error) { + isDubiousShift := func(x, y ast.Expr) (int64, int64, bool) { + typ, ok := pass.TypesInfo.TypeOf(x).(*types.Basic) + if !ok { + return 0, 0, false + } + switch typ.Kind() { + case types.Int8, types.Int16, types.Int32, types.Int64, + types.Uint8, types.Uint16, types.Uint32, types.Uint64: + // We're only interested in fixed–size types. + default: + return 0, 0, false + } + typeSize := pass.TypesSizes.Sizeof(typ) * wordSize + + shiftLength, ok := code.ExprToInt(pass, y) + if !ok { + return 0, 0, false + } + + return typeSize, shiftLength, shiftLength >= typeSize + } + + fn := func(node ast.Node) { + if _, ok := Match(pass, checkFixedLengthTypeShiftQ, node); !ok { + return + } + + switch e := node.(type) { + case *ast.AssignStmt: + for i := range e.Lhs { + if size, shift, yes := isDubiousShift(e.Lhs[i], e.Rhs[i]); yes { + report.Report(pass, e, fmt.Sprintf("shifting %d bits value by %d will always clear it", size, shift)) + } + } + case *ast.BinaryExpr: + if size, shift, yes := isDubiousShift(e.X, e.Y); yes { + report.Report(pass, e, fmt.Sprintf("shifting %d bits value by %d will always clear it", size, shift)) + } + } + } + code.Preorder(pass, fn, (*ast.AssignStmt)(nil), (*ast.BinaryExpr)(nil)) + + return nil, nil +} diff --git a/staticcheck/lint_test.go b/staticcheck/lint_test.go index 7e7fd592d..a628b2dca 100644 --- a/staticcheck/lint_test.go +++ b/staticcheck/lint_test.go @@ -83,6 +83,7 @@ func TestAll(t *testing.T) { "SA9003": {{Dir: "CheckEmptyBranch"}}, "SA9004": {{Dir: "CheckMissingEnumTypesInDeclaration"}}, "SA9005": {{Dir: "CheckNoopMarshal"}}, + "SA9006": {{Dir: "CheckStaticBitShift"}}, } testutil.Run(t, Analyzers, checks) diff --git a/staticcheck/testdata/src/CheckStaticBitShift/CheckStaticBitShift.go b/staticcheck/testdata/src/CheckStaticBitShift/CheckStaticBitShift.go new file mode 100644 index 000000000..efa583308 --- /dev/null +++ b/staticcheck/testdata/src/CheckStaticBitShift/CheckStaticBitShift.go @@ -0,0 +1,87 @@ +package pkg + +// Partially copied from go vet's test suite. + +// Copyright 2014 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE-THIRD-PARTY file. + +func fn() { + var i8 int8 + _ = i8 << 7 + _ = (i8 + 1) << 8 // want `will always clear it` + _ = i8 << (7 + 1) // want `will always clear it` + _ = i8 >> 8 // want `will always clear it` + i8 <<= 8 // want `will always clear it` + i8 >>= 8 // want `will always clear it` + + var i16 int16 + _ = i16 << 15 + _ = i16 << 16 // want `will always clear it` + _ = i16 >> 16 // want `will always clear it` + i16 <<= 16 // want `will always clear it` + i16 >>= 16 // want `will always clear it` + + var i32 int32 + _ = i32 << 31 + _ = i32 << 32 // want `will always clear it` + _ = i32 >> 32 // want `will always clear it` + i32 <<= 32 // want `will always clear it` + i32 >>= 32 // want `will always clear it` + + var i64 int64 + _ = i64 << 63 + _ = i64 << 64 // want `will always clear it` + _ = i64 >> 64 // want `will always clear it` + i64 <<= 64 // want `will always clear it` + i64 >>= 64 // want `will always clear it` + + var u8 uint8 + _ = u8 << 7 + _ = u8 << 8 // want `will always clear it` + _ = u8 >> 8 // want `will always clear it` + u8 <<= 8 // want `will always clear it` + u8 >>= 8 // want `will always clear it` + + var u16 uint16 + _ = u16 << 15 + _ = u16 << 16 // want `will always clear it` + _ = u16 >> 16 // want `will always clear it` + u16 <<= 16 // want `will always clear it` + u16 >>= 16 // want `will always clear it` + + var u32 uint32 + _ = u32 << 31 + _ = u32 << 32 // want `will always clear it` + _ = u32 >> 32 // want `will always clear it` + u32 <<= 32 // want `will always clear it` + u32 >>= 32 // want `will always clear it` + + var u64 uint64 + _ = u64 << 63 + _ = u64 << 64 // want `will always clear it` + _ = u64 >> 64 // want `will always clear it` + u64 <<= 64 // want `will always clear it` + u64 >>= 64 // want `will always clear it` + _ = u64 << u64 +} + +func fn1() { + var ui uint + _ = ui << 64 + _ = ui >> 64 + ui <<= 64 + ui >>= 64 + + var uptr uintptr + _ = uptr << 64 + _ = uptr >> 64 + uptr <<= 64 + uptr >>= 64 + + var i int + _ = i << 64 + _ = i >> 64 + i <<= 64 + i >>= 64 +} From 4aab9d46ac2ed37dc28bd8a37a98aa102b8d7195 Mon Sep 17 00:00:00 2001 From: Alexey Surikov Date: Sat, 16 May 2020 14:03:21 +0200 Subject: [PATCH 2/6] fix and simplify type size computation --- staticcheck/lint.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/staticcheck/lint.go b/staticcheck/lint.go index a1bc5b1ab..3c4fda537 100644 --- a/staticcheck/lint.go +++ b/staticcheck/lint.go @@ -12,7 +12,6 @@ import ( "reflect" "regexp" "regexp/syntax" - "runtime" "sort" "strconv" "strings" @@ -25,7 +24,6 @@ import ( "honnef.co/go/tools/edit" "honnef.co/go/tools/facts" "honnef.co/go/tools/functions" - "honnef.co/go/tools/gcsizes" "honnef.co/go/tools/internal/passes/buildir" "honnef.co/go/tools/internal/sharedcheck" "honnef.co/go/tools/ir" @@ -3826,7 +3824,6 @@ var ( (AssignStmt _ (Or ">>=" "<<=") _) (BinaryExpr _ (Or ">>" "<<") _)) `) - wordSize = gcsizes.ForArch(runtime.GOARCH).WordSize ) func CheckStaticBitShift(pass *analysis.Pass) (interface{}, error) { @@ -3842,14 +3839,16 @@ func CheckStaticBitShift(pass *analysis.Pass) (interface{}, error) { default: return 0, 0, false } - typeSize := pass.TypesSizes.Sizeof(typ) * wordSize + + const bitsInByte = 8 + typeBits := pass.TypesSizes.Sizeof(typ) * bitsInByte shiftLength, ok := code.ExprToInt(pass, y) if !ok { return 0, 0, false } - return typeSize, shiftLength, shiftLength >= typeSize + return typeBits, shiftLength, shiftLength >= typeBits } fn := func(node ast.Node) { From 207619c2cd720f2677a6d8b8b59ff02874fcd25b Mon Sep 17 00:00:00 2001 From: Alexey Surikov Date: Sat, 16 May 2020 14:05:48 +0200 Subject: [PATCH 3/6] different report wording --- staticcheck/lint.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/staticcheck/lint.go b/staticcheck/lint.go index 3c4fda537..a39ef7573 100644 --- a/staticcheck/lint.go +++ b/staticcheck/lint.go @@ -3860,12 +3860,12 @@ func CheckStaticBitShift(pass *analysis.Pass) (interface{}, error) { case *ast.AssignStmt: for i := range e.Lhs { if size, shift, yes := isDubiousShift(e.Lhs[i], e.Rhs[i]); yes { - report.Report(pass, e, fmt.Sprintf("shifting %d bits value by %d will always clear it", size, shift)) + report.Report(pass, e, fmt.Sprintf("shifting %d-bit value by %d bits will always clear it", size, shift)) } } case *ast.BinaryExpr: if size, shift, yes := isDubiousShift(e.X, e.Y); yes { - report.Report(pass, e, fmt.Sprintf("shifting %d bits value by %d will always clear it", size, shift)) + report.Report(pass, e, fmt.Sprintf("shifting %d-bit value by %d bits will always clear it", size, shift)) } } } From f3e5a49f54ddd82272aa80074b3e84b22b52705e Mon Sep 17 00:00:00 2001 From: Alexey Surikov Date: Sat, 16 May 2020 14:11:51 +0200 Subject: [PATCH 4/6] correct bit shifting AST has one Lhs and Rhs --- staticcheck/lint.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/staticcheck/lint.go b/staticcheck/lint.go index a39ef7573..73d46049d 100644 --- a/staticcheck/lint.go +++ b/staticcheck/lint.go @@ -3858,10 +3858,8 @@ func CheckStaticBitShift(pass *analysis.Pass) (interface{}, error) { switch e := node.(type) { case *ast.AssignStmt: - for i := range e.Lhs { - if size, shift, yes := isDubiousShift(e.Lhs[i], e.Rhs[i]); yes { - report.Report(pass, e, fmt.Sprintf("shifting %d-bit value by %d bits will always clear it", size, shift)) - } + if size, shift, yes := isDubiousShift(e.Lhs[0], e.Rhs[0]); yes { + report.Report(pass, e, fmt.Sprintf("shifting %d-bit value by %d bits will always clear it", size, shift)) } case *ast.BinaryExpr: if size, shift, yes := isDubiousShift(e.X, e.Y); yes { From 92b3f832e04a1492d2447a3148c7681db64436ca Mon Sep 17 00:00:00 2001 From: Alexey Surikov Date: Sat, 16 May 2020 14:12:34 +0200 Subject: [PATCH 5/6] typo --- staticcheck/doc.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/staticcheck/doc.go b/staticcheck/doc.go index 47dcd6de2..8a136922a 100644 --- a/staticcheck/doc.go +++ b/staticcheck/doc.go @@ -842,7 +842,7 @@ flag empty structs.`, "SA9006": { Title: `Dubious bit shifting of a fixed size integer value`, - Text: `Bit shifting an value past its size will always clear the value. + Text: `Bit shifting a value past its size will always clear the value. For instance: From 14c2ab092acaa4c97b673d5127729185394917fe Mon Sep 17 00:00:00 2001 From: Alexey Surikov Date: Sat, 16 May 2020 14:14:39 +0200 Subject: [PATCH 6/6] consistent indentation --- staticcheck/doc.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/staticcheck/doc.go b/staticcheck/doc.go index 8a136922a..08f47ffec 100644 --- a/staticcheck/doc.go +++ b/staticcheck/doc.go @@ -846,8 +846,8 @@ flag empty structs.`, For instance: - v := int8(42) - v >>= 8 + v := int8(42) + v >>= 8 will always result in 0. @@ -855,11 +855,11 @@ This check flags bit shifiting operations on fixed size integer values only. That is, int, uint and uintptr are never flagged to avoid potential false positives in somewhat exotic but valid bit twiddling tricks: - // Clear any value above 32 bits if integers are more than 32 bits. - func f(i int) int { - v := i >> 32 - v = v << 32 - return i-v - }`, + // Clear any value above 32 bits if integers are more than 32 bits. + func f(i int) int { + v := i >> 32 + v = v << 32 + return i-v + }`, }, }