Add static analysis for unsafe uint casting #10318

prestonvanloon · 2022-03-07T18:08:23Z

What type of PR is this?

Other

What does this PR do? Why is it needed?

Casting uint types to int types can lead to unexpected results, i.e. overflows.

This PR adds a static analysis check for these scenarios and resolves all found issues.

Which issues(s) does this PR fix?

Other notes for review

One thing I thought about is that it's technically not a big deal to cast a smaller uint to a larger int (uint32 -> int64), but we typically only deal with uint64 so I decided to warn on any conversion from uint to int.

rauljordan · 2022-03-07T18:48:46Z

Note this closes #9958 if all exceptions are taken care of

prestonvanloon · 2022-03-07T21:56:50Z

beacon-chain/p2p/encoder/ssz.go

-	// Defensive check to prevent potential issues when casting to int64.
-	if length > math.MaxInt64 {
-		return 0, errors.Errorf("invalid length provided: %d", length)
+	il, err := math.Int(length)


This validation has moved to math.Int

prestonvanloon · 2022-03-07T22:13:51Z

proto/prysm/v1alpha1/attestation/aggregation/testing/BUILD.bazel

@@ -2,6 +2,7 @@ load("@prysm//tools/go:def.bzl", "go_library")

 go_library(
    name = "go_default_library",
+    testonly = True,


Found this in my journey. More test code not marked as testonly :(

…t-sa

rauljordan

Found a test case the analyzer does not catch: when doing the cast during struct initialization:

func Uint64CastInStruct() {
	type S struct {
		a int
	}
	s := S{
		a: int(uint64(5)), // want "Unsafe cast from uint64 to int."
	}
	_ = s
}

this fails when running the Analyzer test

WORKSPACE

beacon-chain/forkchoice/protoarray/helpers.go

tools/analyzers/uintcast/analyzer_test.go

nisdas · 2022-03-08T14:14:16Z

beacon-chain/cache/active_balance.go

 	// maxBalanceCacheSize defines the max number of active balances can cache.
-	maxBalanceCacheSize = uint64(4)
+	maxBalanceCacheSize = int(4)


This was brought up offline cc @uncdr , but is there any reason we have cache sizes as a signed integer here ? It can't be negative in any case.

Ah k, I see it is due to the lruwrpr, maybe we can change the method signature to take a uint64. And it does the safe casting internally in the lru constructor.

Yeah exactly, I moved these to const so that we are assured it can never change. I thought this was the easier route vs changing 3rd party API

It is not 3rd party though, this lruwrpr package is under prysm.

Oh! OK, I'll take a look!

Why do we even have lruwrpr? It's a flow through to github.com/hashicorp/golang-lru with panics. I'm going to leave it out of scope for this PR. It seems strange to have this package when we could use lru directly.

I think we wanted the lru package to not return an error mostly. The package returns an error if the cache size is negative.

For this, it might be best to handle this in a follow up PR. The LRU cache is used across prysm and changing the api of it, will require changes across prysm

prestonvanloon · 2022-03-08T16:34:19Z

Found a test case the analyzer does not catch: when doing the cast during struct initialization:
func Uint64CastInStruct() {
	type S struct {
		a int
	}
	s := S{
		a: int(uint64(5)), // want "Unsafe cast from uint64 to int."
	}
	_ = s
}
this fails when running the Analyzer test

This is a pretty contrived example. I will see if I can reasonably add to the SA to solve this, but I might not be able to do it in this PR

uncdr

i'm still not ok where int, uint, uint64 should be used

uncdr · 2022-03-08T19:18:51Z

beacon-chain/cache/committee.go

 	// maxCommitteesCacheSize defines the max number of shuffled committees on per randao basis can cache.
 	// Due to reorgs and long finality, it's good to keep the old cache around for quickly switch over.
-	maxCommitteesCacheSize = uint64(32)
+	maxCommitteesCacheSize = int(32)


same as in another thread, why signed int?

That's the 3rd party API. I'm not sure. This is a constant now so it can't overflow/underflow. I think we can address cache API in another PR or issue.

uncdr · 2022-03-08T19:26:57Z

beacon-chain/p2p/message_id.go

@@ -92,7 +92,7 @@ func postAltairMsgID(pmsg *pubsub_pb.Message, fEpoch types.Epoch) string {
 		h := hash.Hash(combinedData)
 		return string(h[:20])
 	}
-	totalLength := len(params.BeaconNetworkConfig().MessageDomainValidSnappy) + len(topicLenBytes) + int(topicLen) + len(decodedData)
+	totalLength := len(params.BeaconNetworkConfig().MessageDomainValidSnappy) + len(topicLenBytes) + int(topicLen) + len(decodedData) // lint:ignore uintcast -- Message length cannot exceed int64


if totalLength exceeds int64? overflow and a negative value?

I'll add a SafeAddInt64 method for this

prestonvanloon · 2022-03-09T13:30:54Z

i'm still not ok where int, uint, uint64 should be used

int and uint are now guaranteed to be 64 bits. Go automatically sets int and uint to the max size available to the system and I have added a runtime check to panic on start if the max size of int/uint is not 64 bits.

In general, int should be used to interact with an API that we do not control.
For example, indexing an array/slice requires an int.

arr[i] // i must be int

There are a few other use cases where int is allowed when negative numbers are expected, like balance deltas in fork choice.

In most cases, uint64 is the canonical type for numbers in Ethereum.

…t-sa

prestonvanloon · 2022-03-10T22:13:48Z

Removing OK to merge until after someone approves this. Since this PR adds new static analysis, there is no cache hits between develop based branches and this one. So this PR is contributing to high build agent disk usage until it can be merged.

* Add static analysis for unsafe uint casting * Fix violations of uintcast * go mod tidy * Add exclusion to nogo for darwin build * Add test for math.Int * Move some things to const so they are assured not to exceed int64 * Self review * lint * fix tests * fix test * Add init check for non 64 bit OS * Move new deps from WORKSPACE to deps.bzl * fix bazel build for go analysis runs * Update BUILD.bazel Remove TODO * add math.AddInt method * Add new test casts * Add case where builtin functions and declared functions are covered * Fix new findings * cleanup Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com> Co-authored-by: Nishant Das <nishdas93@gmail.com>

Add static analysis for unsafe uint casting

d92031f

prestonvanloon added 4 commits March 7, 2022 15:30

Fix violations of uintcast

133de3e

Merge branch 'develop' into uintcast-sa

3070974

go mod tidy

907acc0

Add exclusion to nogo for darwin build

cf72314

prestonvanloon commented Mar 7, 2022

View reviewed changes

prestonvanloon added 2 commits March 7, 2022 16:05

Add test for math.Int

9f2f5b5

Move some things to const so they are assured not to exceed int64

9c6c96a

prestonvanloon commented Mar 7, 2022

View reviewed changes

Self review

5363873

prestonvanloon marked this pull request as ready for review March 7, 2022 22:18

prestonvanloon requested review from rkapka, nisdas and a team as code owners March 7, 2022 22:18

prestonvanloon requested a review from kasey March 7, 2022 22:18

prestonvanloon added 2 commits March 7, 2022 16:26

lint

e139cf0

fix tests

0c4149b

prestonvanloon added OK to merge Security Security Related Issues labels Mar 7, 2022

prestonvanloon linked an issue Mar 7, 2022 that may be closed by this pull request

Remove Unnecessary Use of Integer Types in Prysm #9958

Closed

2 tasks

prestonvanloon added 2 commits March 7, 2022 17:21

fix test

c9fb2d8

Merge branch 'develop' of github.com:prysmaticlabs/prysm into uintcas…

c256d4e

…t-sa

prestonvanloon requested a review from uncdr March 7, 2022 23:31

rauljordan reviewed Mar 8, 2022

View reviewed changes

WORKSPACE Outdated Show resolved Hide resolved

beacon-chain/forkchoice/protoarray/helpers.go Show resolved Hide resolved

tools/analyzers/uintcast/analyzer_test.go Show resolved Hide resolved

nisdas reviewed Mar 8, 2022

View reviewed changes

Merge refs/heads/develop into uintcast-sa

5416ae2

Add init check for non 64 bit OS

279ab22

uncdr reviewed Mar 8, 2022

View reviewed changes

add math.AddInt method

b7288a4

prestonvanloon and others added 15 commits March 9, 2022 07:51

Add new test casts

41385a3

Add case where builtin functions and declared functions are covered

aae7d32

Fix new findings

083dc61

Merge branch 'develop' of github.com:prysmaticlabs/prysm into uintcas…

c41d455

…t-sa

Merge refs/heads/develop into uintcast-sa

f60c8d5

Merge refs/heads/develop into uintcast-sa

677bb93

Merge refs/heads/develop into uintcast-sa

ccce09a

Merge refs/heads/develop into uintcast-sa

9bb6be1

Merge refs/heads/develop into uintcast-sa

b0a7059

Merge refs/heads/develop into uintcast-sa

6c55ae9

Merge refs/heads/develop into uintcast-sa

4d19385

Merge refs/heads/develop into uintcast-sa

483a093

Merge refs/heads/develop into uintcast-sa

f1f1689

Merge refs/heads/develop into uintcast-sa

731a687

Merge refs/heads/develop into uintcast-sa

108cbae

prestonvanloon removed the OK to merge label Mar 10, 2022

nisdas added 3 commits March 11, 2022 13:04

Merge branch 'develop' into uintcast-sa

5c709ff

cleanup

c6f3d53

Merge branch 'develop' into uintcast-sa

22205fd

nisdas added the OK to merge label Mar 11, 2022

nisdas approved these changes Mar 11, 2022

View reviewed changes

Merge refs/heads/develop into uintcast-sa

0c23e3b

prylabs-bulldozer bot merged commit c1197d7 into develop Mar 11, 2022

delete-merged-branch bot deleted the uintcast-sa branch March 11, 2022 09:34

rauljordan mentioned this pull request Mar 14, 2022

Replace a Few More IntFlags with Uint64Flags #10214

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add static analysis for unsafe uint casting #10318

Add static analysis for unsafe uint casting #10318

prestonvanloon commented Mar 7, 2022 •

edited

Loading

rauljordan commented Mar 7, 2022

prestonvanloon Mar 7, 2022

prestonvanloon Mar 7, 2022

rauljordan left a comment

nisdas Mar 8, 2022

nisdas Mar 8, 2022

prestonvanloon Mar 8, 2022

nisdas Mar 8, 2022 •

edited

Loading

prestonvanloon Mar 8, 2022

prestonvanloon Mar 8, 2022

nisdas Mar 8, 2022

nisdas Mar 11, 2022

prestonvanloon commented Mar 8, 2022

uncdr left a comment

uncdr Mar 8, 2022

prestonvanloon Mar 8, 2022

uncdr Mar 8, 2022

prestonvanloon Mar 8, 2022

prestonvanloon commented Mar 9, 2022

prestonvanloon commented Mar 10, 2022

Add static analysis for unsafe uint casting #10318

Add static analysis for unsafe uint casting #10318

Conversation

prestonvanloon commented Mar 7, 2022 • edited Loading

rauljordan commented Mar 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rauljordan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nisdas Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prestonvanloon commented Mar 8, 2022

uncdr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prestonvanloon commented Mar 9, 2022

prestonvanloon commented Mar 10, 2022

prestonvanloon commented Mar 7, 2022 •

edited

Loading

nisdas Mar 8, 2022 •

edited

Loading