search

dotnet-websharper / owin

Wrappers for hosting WebSharper sitelets and remoting components in OWIN projects
Apache License 2.0
10 stars 9 forks source link

Strange hanging with HttpMultipartParser #25

Open nbevans opened 5 years ago

nbevans commented 5 years ago

Anyone ever seen this before? The HttpMultipartParser is hanging on multiple threads (presumably requests that came in) and is spinning the CPU. I managed to pull the following stack trace from our prod server process using Sysinternals Process Explorer.

HttpMultipartParser.dll!HttpMultipartParser.RebufferableBinaryReader.StreamData+0x21
HttpMultipartParser.dll!HttpMultipartParser.RebufferableBinaryReader.ReadByteLine+0x4d
HttpMultipartParser.dll!HttpMultipartParser.MultipartFormDataParser.ParseParameterPart+0xc4
HttpMultipartParser.dll!HttpMultipartParser.MultipartFormDataParser.ParseSection+0x38d
HttpMultipartParser.dll!HttpMultipartParser.MultipartFormDataParser.Parse+0x4d
HttpMultipartParser.dll!HttpMultipartParser.MultipartFormDataParser..ctor+0x1b4
WebSharper.Owin.dll!O2W.ParseFormData+0xb0
WebSharper.Owin.dll!Request@252.Invoke+0x1c
WebSharper.Owin.dll!WebSharper.Owin.EnvKey.GetOrSet+0x91
WebSharper.Owin.dll!WebSharper.Owin.Internal.dispatch+0x43
WebSharper.Owin.dll!siteletAppFunc@566.Invoke+0x8d
Microsoft.Owin.StaticFiles.dll!Microsoft.Owin.StaticFiles.StaticFileMiddleware.Invoke+0x17b
Microsoft.Owin.StaticFiles.dll!Microsoft.Owin.StaticFiles.StaticFileMiddleware.Invoke+0x17b
Microsoft.Owin.StaticFiles.dll!Microsoft.Owin.StaticFiles.StaticFileMiddleware.Invoke+0x17b
Microsoft.Owin.StaticFiles.dll!Microsoft.Owin.StaticFiles.StaticFileMiddleware.Invoke+0x17b
Microsoft.Owin.StaticFiles.dll!Microsoft.Owin.StaticFiles.StaticFileMiddleware.Invoke+0x17b
Microsoft.Owin.StaticFiles.dll!Microsoft.Owin.StaticFiles.StaticFileMiddleware.Invoke+0x17b
Microsoft.Owin.StaticFiles.dll!Microsoft.Owin.StaticFiles.StaticFileMiddleware.Invoke+0x17b
Microsoft.Owin.StaticFiles.dll!Microsoft.Owin.StaticFiles.StaticFileMiddleware.Invoke+0x17b
Microsoft.Owin.StaticFiles.dll!Microsoft.Owin.StaticFiles.StaticFileMiddleware.Invoke+0x17b
Microsoft.Owin.StaticFiles.dll!Microsoft.Owin.StaticFiles.StaticFileMiddleware.Invoke+0x17b
Microsoft.Owin.StaticFiles.dll!Microsoft.Owin.StaticFiles.StaticFileMiddleware.Invoke+0x17b
Microsoft.Owin.StaticFiles.dll!Microsoft.Owin.StaticFiles.StaticFileMiddleware.Invoke+0x17b
Microsoft.Owin.StaticFiles.dll!Microsoft.Owin.StaticFiles.StaticFileMiddleware.Invoke+0x17b
Microsoft.Owin.StaticFiles.dll!Microsoft.Owin.StaticFiles.StaticFileMiddleware.Invoke+0x17b
Microsoft.Owin.StaticFiles.dll!Microsoft.Owin.StaticFiles.StaticFileMiddleware.Invoke+0x17b
Microsoft.Owin.StaticFiles.dll!Microsoft.Owin.StaticFiles.StaticFileMiddleware.Invoke+0x17b
Singapore.Core.Util.Owin.dll!Invoke@216-1.Invoke+0x9c
nbevans commented 5 years ago

https://github.com/Vodurden/Http-Multipart-Data-Parser/issues/29

It seems this fix hasn’t been merged into WebSharper.

nbevans commented 5 years ago

It seems that HttpMultipartParser lib may be ripe for replacement with say MimeKitLite. This is a far better tested and production grade lib that can easily handle multipart parsing.

https://www.nuget.org/packages/MimeKitLite/

granicz commented 5 years ago

Thanks for digging into this. I agree, HMP looks a bit outdated - replacing would come with no extra dependencies and would remove the need for custom code in this repo. Would you like to give it a go and submit a PR?

nbevans commented 5 years ago

Hi @granicz

I needed to fix it on our production app super urgently so I imported the WebSharper.Owin (really it is just one .fs file) code to our repo, and tweaked it to use the latest NuGet of HttpMultipartParser. This seemed to all go very well and we've deployed this now so will see if it fixes the immediate problem we were having.

It seems the original reason HMP was imported into the WebSharper.Owin repo was because it needed to add the "leaveOpen" support to the Stream. I worked around this by using my NonClosingStream (see below) wrapper/decorator - which means I didn't need to make any tweaks to HMP's source code. I'm not sure why WebSharper.Owin didn't just do this originally? Bleh...

I still think HMP needs replacing though. With no offence intended to the original author of it, it hasn't had a commit/release for a couple years now, and there are alternative libraries nowadays like MimeKitLite which are better maintained and more suitable for use on production grade systems - particularly because of better edge case handling thus reducing possible attack vectors.

I am investigating what needs doing to rip out HMP and replace with MimeKitLite.


/// Stream decorator that swallows Dispose() calls.
/// Optionally, can reset the position of a seekable stream to the start when disposed.
type NonClosingStream(stream:Stream, ?resetPositionOnDispose) =
    inherit Stream()

    override __.CanRead = stream.CanRead
    override __.CanSeek = stream.CanSeek
    override __.CanWrite = stream.CanWrite
    override __.Length = stream.Length
    override __.Position with get() = stream.Position and set(v) = stream.Position <- v
    override __.Flush() = stream.Flush()
    override __.Seek(offset, origin) = stream.Seek(offset, origin)
    override __.SetLength(v) = stream.SetLength(v)
    override __.Read(buffer, offset, count) = stream.Read(buffer, offset, count)
    override __.Write(buffer, offset, count) = stream.Write(buffer, offset, count)
    override __.Dispose _ = if defaultArg resetPositionOnDispose false && stream.CanSeek then stream.Position <- 0L
granicz commented 5 years ago

Sounds good, thanks!

nbevans commented 4 years ago

https://gist.github.com/nbevans/219a65ed04cc8c401a390e4acef70bdd

I forgot to come back and post this. My fixes went a bit above & beyond just the MIME parsing.