Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HTTP manager shared state corruption when sending wrong length for stream #204

Closed
sanketr opened this issue Jun 29, 2016 · 5 comments
Closed

HTTP manager shared state corruption when sending wrong length for stream #204

sanketr opened this issue Jun 29, 2016 · 5 comments

Comments

@sanketr
Copy link

sanketr commented Jun 29, 2016

Cross-post from SO where I asked about it.

Given a shared HTTP manager, it seems that if the requestBody is of type requestBodySource and if wrong length is supplied for the request body, then subsequent requests will crap out on same HTTP manager for about 20+ seconds (about 20 seconds in aws package). There seems to be something about interaction of shared state and GivesPopper perhaps that is causing this issue. Here is a sample code that reproduces it - we use requestb.in for sending wrong length upload, and then try to read another valid URL on requestb.in.

{-# LANGUAGE OverloadedStrings #-}

    import           Data.Conduit.Binary (sourceFile)
    import           Network.HTTP.Conduit 
    import           Network.HTTP.Types
    import qualified Data.ByteString.Lazy as LBS
    import System.IO
    import Control.Monad.Trans.Resource (runResourceT)
    import Control.Concurrent.Async (async,waitCatch)
    import Control.Exception (displayException)

    main :: IO ()
    main = do
      {- Set up a ResourceT region with an available HTTP manager. -}
      httpmgr <- newManager tlsManagerSettings
      httpmgr2 <- newManager tlsManagerSettings
      let file ="out" -- some byte contents with length > 1
      lenb <- System.IO.withFile file ReadMode hFileSize
      let inbytes = sourceFile file 
      initReq <- parseUrl "http://requestb.in/saxbx3sa"
      putreq <- async $ runResourceT $ do
        let req = initReq { method = "POST",
          -- let us send wrong length in requestBodySource
          requestBody = (requestBodySource (fromIntegral $ lenb - 1) inbytes)}
        resp <- httpLbs req httpmgr 
        return (statusCode . responseStatus $ resp)
      putreqRes <- waitCatch putreq
      case putreqRes of
        Left e -> print $ displayException $ e
        Right r -> print $ r
      getreq <- async $ runResourceT $ do
        -- Let us do a GET on a different resource to see if it works
        initReq <- parseUrl "http://requestb.in/1l15sz21"
        let req = initReq { method = "GET"}
        resp <- httpLbs req httpmgr 
        return (statusCode . responseStatus $ resp)
      getreqRes <- waitCatch getreq
      case getreqRes of
        Left e -> print $ displayException $ e
        Right r -> print $ r

Output - first bad upload goes through as HTTP 200, and subsequent GET request immediately causes HTTP 400 error - sometimes, it will be HTTP 403 instead:

  *Main> main
        200
        "StatusCodeException (Status {statusCode = 400, statusMessage = \"Bad Request\"})
     [(\"Date\",\"Wed, 29 Jun 2016 11:54:59 GMT\"),(\"Content-Type\",\"text/html\"),
    (\"Content-Length\",\"177\"),(\"Connection\",\"close\"),(\"Server\",\"-nginx\"),
    (\"CF-RAY\",\"-\"),(\"X-Response-Body-Start\",\"<html>\\r\\n<head><title>400 Bad 
    Request</title></head>\\r\\n<body bgcolor=\\\"white\\\">\\r\\n<center><h1>400 Bad 
    Request</h1></center>\\r\\n<hr><center>cloudflare-
    nginx</center>\\r\\n</body>\\r\\n</html>\\r\\n\"),(\"X-Request-URL\",\"GET 
    http://requestb.in:80/saxbx3sa\")] (CJ {expose = []})"

On wrong streaming length, the behavior shouldn't be to corrupt the HTTP manager as seems to happen here.

I have also simulated a source file for requestBodySource where length is correct, but the source aborts mid-way due to a simulated failure (to simulate network issues). In that case, there are no errors. So, it seems we have just one case where sending wrong length without any failures will cause some kind of shared state to become corrupt here, which gets released within 25 seconds in aws package (might be some kind of timeout setting in there).
@sanketr sanketr mentioned this issue Jun 29, 2016
Open
@snoyberg
Copy link
Owner

I haven't had a chance to try your example, but this behavior doesn't surprise me: the current code doesn't actually confirm that the caller is passing in valid parameters IIRC. Adding in such a check should not be too difficult, and would be in the Network.HTTP.Client.Request module. Would you be interested in taking a stab at this?

@sanketr
Copy link
Author

sanketr commented Jun 29, 2016

Gotcha. One question though - exactly where the corruption might be occurring? It will be good to know. This issue makes the HTTP manager go in bad state, and reject all subsequent requests until it recovers.

So, I have the same workaround of validating the length. Before I take a stab, would a solution like below be suitable for adaptation? Basically, we need to keep track of length in source stream, and return some kind of failure if length doesn't match the passed in parameter.

sourceMsgWLen :: (MonadIO m,MonadThrow m) => Connection -> Int -> C.Source m BS.ByteString
sourceMsgWLen conn len = go 0 where 
    go n = do
      bs <-  liftIO $ receiveData conn
      -- keep processing until we receive end of message - last chunk will
      -- be empty
      if (not . BS.null $ val) then do
         C.yield $ val
         go (n + (BS.length val))
       else if (len /= n) then throwM $ ChunkException "Data was sent with wrong size"
         else return ()

@snoyberg
Copy link
Owner

Yeah, something like that makes sense. It's not actually the manager itself that's getting corrupted, it's the connection to the server. It seems that the beginning of the next request is being treated as the end of the previous request body, causing the next request to be misinterpreted by the server.

@sanketr
Copy link
Author

sanketr commented Jun 29, 2016

Thanks for explanation.

I am going to take a stab at fixing this now. I have identified the code block to be fixed. Before I do a fix and PR, could someone please comment on length calculation in case isChunked is true. Should it still be BS.length? Just confirming.

Also, there is matter of what kind of error to raise here. Perhaps HttpParserException? Or StatusCode with status 400 (bad content)? Or anything better?

@sanketr sanketr mentioned this issue Jun 29, 2016
Merged
@snoyberg
Copy link
Owner

Looks like this was resolved by #205.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@snoyberg @sanketr