From d4952dc0d6cf7e71d59586823cfda3010a18a5b1 Mon Sep 17 00:00:00 2001 From: Emmanuel T Odeke Date: Tue, 20 Sep 2022 16:33:22 -0700 Subject: [PATCH] fix: x/gov/client/cli: use strconv.ParseInt, update & tighten Prompt.Parse tests Uses strconv.ParseInt(s, 10, 0) where 0 as the bitSize lets the system determine the bitSize to parse int into its range. Adds more rigorous checks to ensure that the output is as expected with range failures. While here also added control tests to ensure that we can parse integers within range of int. Updates #13346 --- x/gov/client/cli/prompt.go | 6 ++-- x/gov/client/cli/prompt_test.go | 61 ++++++++++++++++++++++++++++----- 2 files changed, 56 insertions(+), 11 deletions(-) diff --git a/x/gov/client/cli/prompt.go b/x/gov/client/cli/prompt.go index faf7d352e19d..337808bb3ea7 100644 --- a/x/gov/client/cli/prompt.go +++ b/x/gov/client/cli/prompt.go @@ -4,7 +4,7 @@ import ( "encoding/json" "fmt" "os" - "reflect" + "reflect" // #nosec "sort" "strconv" "strings" @@ -94,7 +94,7 @@ func Prompt[T any](data T, namePrefix string) (T, error) { case reflect.String: v.Field(i).SetString(result) case reflect.Int: - resultInt, err := strconv.Atoi(result) + resultInt, err := strconv.ParseInt(result, 10, 0) if err != nil { return data, fmt.Errorf("invalid value for int: %w", err) } @@ -104,7 +104,7 @@ func Prompt[T any](data T, namePrefix string) (T, error) { // [minInt64, maxInt64] // of which on 64-bit machines, which are most common, // int==int64 - v.Field(i).SetInt(int64(resultInt)) + v.Field(i).SetInt(resultInt) default: // skip other types // possibly in the future we can add more types (like slices) diff --git a/x/gov/client/cli/prompt_test.go b/x/gov/client/cli/prompt_test.go index 2629cc65301b..cdf4b4249fd8 100644 --- a/x/gov/client/cli/prompt_test.go +++ b/x/gov/client/cli/prompt_test.go @@ -1,26 +1,33 @@ +//go:build !race +// +build !race + +// Disabled -race because the package github.com/manifoldco/promptui@v0.9.0 +// has a data race and this code exposes it, but fixing it would require +// holding up the associated change to this. + package cli_test import ( - "sync" + "fmt" + "math" + "os" "testing" "github.com/chzyer/readline" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/cosmos/cosmos-sdk/x/gov/client/cli" ) type st struct { - ToOverflow int + I int } -// On the tests running in Github actions, somehow there are races. -var globalMu sync.Mutex - // Tests that we successfully report overflows in parsing ints // See https://github.com/cosmos/cosmos-sdk/issues/13346 func TestPromptIntegerOverflow(t *testing.T) { - // Intentionally sending a value out of the range of + // Intentionally sending values out of the range of int. intOverflowers := []string{ "-9223372036854775809", "9223372036854775808", @@ -33,11 +40,49 @@ func TestPromptIntegerOverflow(t *testing.T) { for _, intOverflower := range intOverflowers { overflowStr := intOverflower t.Run(overflowStr, func(t *testing.T) { - readline.Stdout.Write([]byte(overflowStr + "\n")) + origStdin := readline.Stdin + defer func() { + readline.Stdin = origStdin + }() + + fin, fw := readline.NewFillableStdin(os.Stdin) + readline.Stdin = fin + fw.Write([]byte(overflowStr + "\n")) v, err := cli.Prompt(st{}, "") - assert.NotNil(t, err, "expected a report of an overflow") assert.Equal(t, st{}, v, "expected a value of zero") + require.NotNil(t, err, "expected a report of an overflow") + require.Contains(t, err.Error(), "range") + }) + } +} + +func TestPromptParseInteger(t *testing.T) { + // Intentionally sending a value out of the range of + values := []struct { + in string + want int + }{ + {fmt.Sprintf("%d", math.MinInt), math.MinInt}, + {"19991", 19991}, + {"991000000199", 991000000199}, + } + + for _, tc := range values { + tc := tc + t.Run(tc.in, func(t *testing.T) { + origStdin := readline.Stdin + defer func() { + readline.Stdin = origStdin + }() + + fin, fw := readline.NewFillableStdin(os.Stdin) + readline.Stdin = fin + fw.Write([]byte(tc.in + "\n")) + + v, err := cli.Prompt(st{}, "") + assert.Nil(t, err, "expected a nil error") + assert.Equal(t, tc.want, v.I, "expected a value of zero") }) } }