Avoid using Opt.auto to avoid overflows going silent #864

smelc · 2024-08-08T16:00:14Z

Changelog

- description: |
    Forbid incorrect values in parsers of many Int-like options. Previously those values would overflow and be turned into a random valid value. If this breaks your use case, this means your use case wasn't doing what you expected.
# uncomment types applicable to the change:
  type:
  # - feature        # introduces a new feature
  # - breaking       # the API has changed in a breaking way
  # - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
  # - refactoring    # QoL changes
  - bugfix         # fixes a defect
  # - test           # fixes/modifies tests
  # - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

Context

We use auto a lot in parsing. auto relies under the hood on Read instances which are known to have problems. Some instances accept wrong values and return out of thin air values. This PR fixes this.

Fixes #860

How to trust this PR

Try this PR with cardano-testnet: tested cabal test cardano-testnet-test with cardano-node at e9ab511272a7694196778007211991a9984557e8 and cardano-cli at 44ca6ed
Run git grep auto "*.hs" and observe that all the remaining calls are in a byron file (I didn't change those)

Checklist

Commit sequence broadly makes sense and commits have useful messages
New tests are added if needed and existing tests are updated. See Running tests for more details
Self-reviewed the diff

cardano-cli/src/Cardano/CLI/EraBased/Options/Common.hs

carbolymer

I like the direction, but wordReader needs some polishing. 😃

smelc · 2024-08-09T15:27:31Z

@carbolymer> I did the changes 👍

Given your last comment, I assume you're fine with me deploying this to many more code locations? (please don't approve though yet, because I'll extend this PR).

cardano-cli/src/Cardano/CLI/EraBased/Options/Common.hs

Jimbo4350

LGTM! Now just replace all calls to auto.

cardano-cli/src/Cardano/CLI/EraBased/Options/Common.hs

smelc · 2024-08-22T15:39:57Z

cardano-cli/src/Cardano/CLI/EraBased/Options/Common.hs

+pairIntegralReader :: (Typeable a, Integral a, Bits a) => ReadM (a, a)
+pairIntegralReader = readerFromParsecParser pairIntegralParsecParser
+
+pairIntegralParsecParser :: (Typeable a, Integral a, Bits a) => Parsec.Parser (a, a)


This one is important to review

looks fine to me 👌🏻

Jimbo4350 · 2024-08-23T18:15:22Z

cardano-cli/test/cardano-cli-test/Test/Cli/Parser.hs

+-- @cabal test cardano-cli-test --test-options '-p "/pair integral reader/"'@
+hprop_pair_integral_reader :: Property
+hprop_pair_integral_reader = propertyOnce $ do
+  assert $ isRight $ parse @Word "(0, 0)"


We can improve these tests with hedgehog generators. Example:

diff --git a/cardano-cli/test/cardano-cli-test/Test/Cli/Parser.hs b/cardano-cli/test/cardano-cli-test/Test/Cli/Parser.hs index 1bf19f8b8..c72c8a093 100644 --- a/cardano-cli/test/cardano-cli-test/Test/Cli/Parser.hs +++ b/cardano-cli/test/cardano-cli-test/Test/Cli/Parser.hs @@ -1,3 +1,5 @@ +{-# LANGUAGE RankNTypes #-} +{-# LANGUAGE ScopedTypeVariables #-} {-# LANGUAGE TypeApplications #-} module Test.Cli.Parser @@ -12,11 +14,14 @@ import Cardano.CLI.EraBased.Options.Common (integralParsecParser, import Data.Bits (Bits) import Data.Data (Typeable) import Data.Either (isLeft, isRight) +import Data.Proxy import Data.Word (Word16) import qualified Text.Parsec as Parsec -import Hedgehog (Property, assert) +import Hedgehog import Hedgehog.Extras (propertyOnce) +import qualified Hedgehog.Gen as Gen +import qualified Hedgehog.Range as Range -- | Execute me with: -- @cabal test cardano-cli-test --test-options '-p "/integral reader/"'@ @@ -45,9 +50,8 @@ hprop_integral_reader = propertyOnce $ do -- @cabal test cardano-cli-test --test-options '-p "/pair integral reader/"'@ hprop_pair_integral_reader :: Property hprop_pair_integral_reader = propertyOnce $ do - assert $ isRight $ parse @Word "(0, 0)" - assert $ isRight $ parse @Word " ( 0 , 0 )" - assert $ isRight $ parse @Word " (18446744073709551615 , 18446744073709551614 ) " + validArbitraryTuple <- forAll $ genNumberTuple (Proxy :: Proxy Word) + assert $ isRight $ parse @Word validArbitraryTuple assert $ isLeft $ parse @Word "(0, 0, 0)" assert $ isLeft $ parse @Word "(-1, 0)" @@ -64,3 +68,18 @@ hprop_pair_integral_reader = propertyOnce $ do case Parsec.runParser pairIntegralParsecParser () "" s of Left parsecError -> Left $ show parsecError Right x -> Right x + +genNumberTuple + :: forall integral + . Integral integral + => Show integral + => Proxy integral -> Gen String +genNumberTuple _ = do + x :: integral <- Gen.integral (Range.linear 0 100) + y :: integral <- Gen.integral (Range.linear 0 100) + space1 <- genArbitrarySpace + space2 <- genArbitrarySpace + return $ "(" ++ space2 ++ show x ++ space1 ++ "," ++ space2 ++ show y ++ space1 ++ ")" + +genArbitrarySpace :: Gen String +genArbitrarySpace = Gen.string (Range.linear 0 5) (return ' ')

We should as much as possible avoid unit tests.

I took your suggestion and split the function into two, since the positive tests need to be run with property, while the negative tests need to run with propertyOnce 👍

while the negative tests need to run with propertyOnce

Why? You could also write a generator to inject a random character (or multiple characters) between the brackets for the negative tests.

Yes but I would need to rule out generators that would still yield a valid value (e.g. if the generated character is a number) and I believe writing a good generator for negative tests it overkill for testing this parser. So I'll stick to the manual negative tests to speed up merging.

Jimbo4350

LGTM however you can improve your tests further using hedgehog generators.

Jimbo4350 · 2024-08-26T16:36:01Z

cardano-cli/test/cardano-cli-test/Test/Cli/Parser.hs

+-- @cabal test cardano-cli-test --test-options '-p "/integral reader/"'@
+hprop_integral_reader :: Property
+hprop_integral_reader = propertyOnce $ do
+  assert $ isRight $ parse @Word "0"


You should use https://hackage.haskell.org/package/hedgehog-1.5/docs/Hedgehog-Internal-Gen.html#v:word instead.

👍 Done with:

w <- forAll $ Gen.word $ Gen.linear minBound maxBound assert $ isRight $ parse @Word (show w)

Jimbo4350 · 2024-08-26T16:37:20Z

cardano-cli/test/cardano-cli-test/Test/Cli/Parser.hs

+  validArbitraryTuple <- forAll $ genNumberTuple (Proxy :: Proxy Word)
+  assert $ isRight $ parse @Word validArbitraryTuple
+
+  assert $ isLeft $ parse @Word "(0, 0, 0)"


You can write a generator that injects a random character (or characters) into the string generated by genNumberTuple. This would provide better coverage.

Jimbo4350 · 2024-08-26T16:38:18Z

cardano-cli/test/cardano-cli-test/Test/Cli/Parser.hs

+  assert $ isLeft $ parse @Word "(0, 0, 0)"
+  assert $ isLeft $ parse @Word "(-1, 0)"
+  assert $ isLeft $ parse @Word "(18446744073709551616, 0)"
+  assert $ isLeft $ parse @Word "(0, 18446744073709551616)"


These are already tested in hprop_integral_pair_reader_negative

Ooops, good catch. Correcting 👍

carbolymer · 2024-08-27T13:49:40Z

cardano-cli/test/cardano-cli-test/Test/Cli/Parser.hs

+hprop_integral_reader :: Property
+hprop_integral_reader = property $ do
+  assert $ isRight $ parse @Word "0"
+  assert $ isRight $ parse @Word "42"


it would be better to check the exact value in case of Rights here

Suggested change

assert $ isRight $ parse @Word "42"

parse @Word "42" === Right 42

Good point, changing 👍

carbolymer · 2024-08-27T13:55:13Z

cardano-cli/test/cardano-cli-test/Test/Cli/Parser.hs

+  assert $ isRight $ parse @Word "0"
+  assert $ isRight $ parse @Word "42"
+  assert $ isLeft $ parse @Word "-1"
+  assert $ isLeft $ parse @Word "18446744073709551616"


It would be better to write this as

Suggested change

assert $ isLeft $ parse @Word "18446744073709551616"

assertWith isLeft $ parse @Word "18446744073709551616"

assert does not report the value on assertion failure

Changed to (for example) assertWith (parse @Word "18446744073709551616") isLeft

carbolymer

LGTM

smelc commented Aug 8, 2024

View reviewed changes