Merge pull request #1229 from input-output-hk/KtorZ/1228/safe-eitherR…

…unGet

Define and use safe 'eitherRunGet' to avoid throwing on binary decoding

lib/core/cardano-wallet-core.cabal

@@ -139,6 +139,7 @@ library
       Cardano.Wallet.Unsafe
       Cardano.Wallet.Version
       Cardano.Wallet.Version.TH
+      Data.Binary.Get.Safe
       Data.Function.Utils
       Data.Time.Text
       Data.Time.Utils

lib/core/src/Cardano/Wallet/DaedalusIPC.hs

@@ -34,6 +34,10 @@ import Control.Exception
     ( IOException, catch, tryJust )
 import Control.Monad
     ( forever )
+import Control.Monad.Trans.Class
+    ( lift )
+import Control.Monad.Trans.Except
+    ( ExceptT, except, runExceptT )
 import Data.Aeson
     ( FromJSON (..)
     , ToJSON (..)
@@ -48,7 +52,9 @@ import Data.Aeson
 import Data.Bifunctor
     ( first )
 import Data.Binary.Get
-    ( getWord32le, getWord64le, runGet )
+    ( getWord32le, getWord64le )
+import Data.Binary.Get.Safe
+    ( eitherRunGet )
 import Data.Binary.Put
     ( putLazyByteString, putWord32le, putWord64le, runPut )
 import Data.Maybe
@@ -197,36 +203,39 @@ ipcListener handle hello onMsg = do
     replyLoop = forever (recvMsg >>= onMsg >>= maybeSend)
 
     recvMsg :: IO (Either Text msgin)
-    recvMsg = first T.pack . eitherDecode <$> readMessage handle
+    recvMsg = fmap (first T.pack) $ (>>= eitherDecode) <$> readMessage handle
 
     sendMsg :: msgout -> IO ()
     sendMsg = sendMessage handle . encode
 
     maybeSend :: Maybe msgout -> IO ()
     maybeSend = maybe (pure ()) sendMsg
 
-readMessage :: Handle -> IO BL.ByteString
-readMessage = if isWindows then windowsReadMessage else posixReadMessage
+readMessage :: Handle -> IO (Either String BL.ByteString)
+readMessage =
+    if isWindows
+    then windowsReadMessage
+    else fmap Right . posixReadMessage
 
 isWindows :: Bool
 isWindows = os == "mingw32"
 
-windowsReadMessage :: Handle -> IO BL.ByteString
-windowsReadMessage handle = do
+windowsReadMessage :: Handle -> IO (Either String BL.ByteString)
+windowsReadMessage handle = runExceptT $ do
     _int1 <- readInt32 handle
     _int2 <- readInt32 handle
-    size <- readInt64 handle
-    BL.hGet handle $ fromIntegral size
+    size  <- readInt64 handle
+    lift $ BL.hGet handle $ fromIntegral size
   where
-    readInt64 :: Handle -> IO Word64
+    readInt64 :: Handle -> ExceptT String IO Word64
     readInt64 hnd = do
-        bs <- BL.hGet hnd 8
-        pure $ runGet getWord64le bs
+        bs <- lift $ BL.hGet hnd 8
+        except $ eitherRunGet getWord64le bs
 
-    readInt32 :: Handle -> IO Word32
+    readInt32 :: Handle -> ExceptT String IO Word32
     readInt32 hnd = do
-        bs <- BL.hGet hnd 4
-        pure $ runGet getWord32le bs
+        bs <- lift $ BL.hGet hnd 4
+        except $ eitherRunGet getWord32le bs
 
 posixReadMessage :: Handle -> IO BL.ByteString
 posixReadMessage = fmap L8.pack . hGetLine

lib/core/src/Data/Binary/Get/Safe.hs

@@ -0,0 +1,26 @@
+-- |
+-- Copyright: © 2018-2019 IOHK
+-- License: Apache-2.0
+--
+-- Extra helpers for safe decoding of binary data.
+
+module Data.Binary.Get.Safe
+    ( eitherRunGet
+    ) where
+
+import Prelude
+
+import Data.Binary.Get
+    ( Get, runGetOrFail )
+
+import qualified Data.ByteString.Lazy as BL
+
+
+-- | A safe version of 'runGet' which doesn't throw on error.
+eitherRunGet
+    :: Get a
+    -> BL.ByteString
+    -> Either String a
+eitherRunGet decoder bytes = case runGetOrFail decoder bytes of
+    Right (_, _, a) -> Right a
+    Left  (_, _, e) -> Left  e

lib/jormungandr/cardano-wallet-jormungandr.cabal

@@ -153,6 +153,7 @@ test-suite unit
     , monad-loops
     , QuickCheck
     , safe
+    , servant
     , text
     , text-class
     , time

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Api/Types.hs

@@ -33,7 +33,7 @@ module Cardano.Wallet.Jormungandr.Api.Types
 import Prelude
 
 import Cardano.Wallet.Jormungandr.Binary
-    ( Block, getBlock, runGet )
+    ( Block, eitherRunGet, getBlock )
 import Cardano.Wallet.Primitive.Types
     ( EpochNo (..)
     , Hash (..)
@@ -109,7 +109,7 @@ instance ToHttpApiData AccountId where
 
 instance MimeUnrender JormungandrBinary [BlockId] where
     mimeUnrender _ =
-        pure . fmap (BlockId . Hash) . runGet (many $ getByteString 32)
+        fmap (map (BlockId . Hash)) . eitherRunGet (many $ getByteString 32)
 
 instance MimeUnrender Hex BlockId where
     mimeUnrender _ bs =
@@ -134,7 +134,7 @@ instance Accept JormungandrBinary where
     contentType _ = contentType $ Proxy @Servant.OctetStream
 
 instance MimeUnrender JormungandrBinary Block where
-    mimeUnrender _ = pure . runGet getBlock
+    mimeUnrender _ = eitherRunGet getBlock
 
 instance MimeRender JormungandrBinary SealedTx where
     mimeRender _ (SealedTx bytes) = BL.fromStrict bytes

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Binary.hs

@@ -72,6 +72,7 @@ module Cardano.Wallet.Jormungandr.Binary
       -- * Re-export
     , Get
     , runGet
+    , eitherRunGet
     , runGetOrFail
     , Put
     , runPut
@@ -134,6 +135,8 @@ import Data.Binary.Get
     , runGetOrFail
     , skip
     )
+import Data.Binary.Get.Safe
+    ( eitherRunGet )
 import Data.Binary.Put
     ( Put, PutM, putByteString, putWord16be, putWord64be, putWord8, runPut )
 import Data.Bits

lib/jormungandr/test/unit/Cardano/Wallet/Jormungandr/ApiSpec.hs

@@ -16,8 +16,11 @@ import Cardano.Wallet.Jormungandr.Api.Types
     ( AccountState (..)
     , ApiStakeDistribution (..)
     , ApiT (..)
+    , JormungandrBinary
     , StakeApiResponse (..)
     )
+import Cardano.Wallet.Jormungandr.Binary
+    ( Block )
 import Cardano.Wallet.Primitive.Types
     ( PoolId (..) )
 import Cardano.Wallet.Unsafe
@@ -28,6 +31,8 @@ import Data.Aeson
     ( eitherDecode )
 import Data.Aeson.QQ
     ( aesonQQ )
+import Data.Either
+    ( isLeft )
 import Data.Proxy
     ( Proxy (..) )
 import Data.Quantity
@@ -36,10 +41,12 @@ import Data.Text.Class
     ( ToText (..) )
 import Data.Word
     ( Word64 )
+import Servant.API
+    ( MimeUnrender (..) )
 import Test.Aeson.Internal.RoundtripSpecs
     ( roundtripSpecs )
 import Test.Hspec
-    ( Spec, describe, it, shouldBe )
+    ( Spec, describe, it, shouldBe, shouldSatisfy )
 import Test.QuickCheck
     ( Arbitrary (..) )
 
@@ -192,6 +199,12 @@ spec = do
                 Left "Error in $.stake.unassigned: Word64 is either floating or \
                      \will cause over or underflow: 1.001000023e7"
             return ()
+
+        describe "MimeUnrender decoding" $ do
+            it "returns 'Left' when encountering an invalid binary block" $ do
+                mimeUnrender (Proxy @JormungandrBinary) ""
+                    `shouldSatisfy` (isLeft @_ @Block)
+
   where
     decodeJSON = eitherDecode :: BL.ByteString -> Either String StakeApiResponse