winterland1989 / binary-parsers

Extends binary with parsec/attoparsec style parsing combinators
BSD 3-Clause "New" or "Revised" License
26 stars 8 forks source link

Fix skipN bug by ensuring that no empty bytestring appear #5

Closed jbransen closed 5 years ago

jbransen commented 5 years ago

After a long debugging session with mysql-haskell throwing UnexpectedPacket errors, which came from a rollback hiding the underlying DecodePacketFailed exceptions, I traced this down to mysql sending back data in chunks, triggering a bug in the parsing with lazy bytestrings. To reproduce, look at the following test program:

import Data.Binary
import Data.Binary.Parser
import Data.ByteString
import qualified Data.ByteString.Lazy          as L
import qualified Data.ByteString.Lazy.Internal as LI

data X = X { test :: ByteString } deriving (Show, Eq)

instance Binary X where
    get = X <$  skipN 4
            <*> getLenEncBytes
    put = undefined

success :: IO X
success = res where
  s :: L.ByteString
  s = LI.chunk "abcd" "\EOTtest"
  res = case parseDetailLazy get s of
    Left e -> error $ show e
    Right (_,_,r) -> return r

fails :: IO X
fails = res where
  s :: L.ByteString
  s = LI.chunk "a" "bcd\EOTtest"
  res = case parseDetailLazy get s of
    Left e -> error $ show e
    Right (_,_,r) -> return r

getLenEncBytes :: Get ByteString
getLenEncBytes = getLenEncInt >>= getByteString

getLenEncInt:: Get Int
getLenEncInt = fromIntegral <$> getWord8

With the current master branch the fails function fails, just because the chunk of the lazy bytestring happens to be split at the wrong point. With my fix it works properly, which should solve the issues we were seeing in the mysql binary protocol parsing.

jbransen commented 5 years ago

I now realize that changing skip (l - n) to skip (n - l) would probably be an easier fix, but I do think that mine is safer. The skip (l - n) where l < n is pretty obviously wrong :-)

winterland1989 commented 5 years ago

Thanks!