2

Closed

StackOverflowException at large http message body processing by HttpContentMultipartExtensions

description

We're using nightly nuget packages 2013-03-30. When we're trying to upload large files (for instance > 2Mb) and process it by using HttpContentMultipartExtensions we've got unhandled exception which crashes AppDomain:

"An unhandled exception of type 'System.StackOverflowException' occurred in Unknown Module."

Code sample is straightforward and looks like http://www.asp.net/web-api/overview/working-with-http/sending-html-form-data,-part-2

After research we've found that in MoveToNextSegmentAsync method exists recursive call:
private static async Task<bool> MoveToNextSegmentAsync(MultipartAsyncContext context)
{
    ...
    if (!await MoveToNextSegmentAsync(context))
    {
        await MoveToNextPartAsync(context);
    }
    ...
}
It was commited by phenning on Feb 26, 2013. Commit 83169ad25a4c:
http://aspnetwebstack.codeplex.com/SourceControl/changeset/83169ad25a4cbdd99b5f80009a5ad75e9c69de3e#src/System.Net.Http.Formatting/HttpContentMultipartExtensions.cs

When large http body is processing this code fragment creates a really huge call stack which causes StackOverflowException and AppDomain crash.
Closed Jun 4, 2013 at 5:09 PM by kichalla
Verified.

comments

HenrikN wrote Mar 30, 2013 at 9:14 PM

Do you know whether it repros with the official version 1 bits?

dtretyakov wrote Mar 30, 2013 at 9:27 PM

dtretyakov wrote Mar 30, 2013 at 10:23 PM

Hello, Henrik,

Let me tell a short story.

We caught few issues in load testing of our web application with >100 rps where HttpContentMultipartExtensions throws exceptions in about 1% of requests:
"Error reading MIME multipart body part."
Inner message: "The client disconnected."
It was reproduced in IIS 7.5 & 8 and only without debugging at the local machines and in the Azure Cloud. System.Net.Http.Formatting library was used from latest stable nuget package http://nuget.org/packages/Microsoft.AspNet.WebApi.Client.

After code review of v4 HttpContentMultipartExtensions we found that in this static class exists next suspicious static members:
private static readonly AsyncCallback _onMultipartReadAsyncComplete = new AsyncCallback(OnMultipartReadAsyncComplete);
private static readonly AsyncCallback _onMultipartWriteSegmentAsyncComplete = new AsyncCallback(OnMultipartWriteSegmentAsyncComplete);
As a result we started afraid of this code fragment :) and found a new v5 branch of Web API where HttpContentMultipartExtensions was refactored by phenning.

This version passed our smoke tests without strange "Error reading MIME multipart body part." exceptions, but our qa team tests was not passed because they use much more larger files.

HenrikN wrote Mar 31, 2013 at 1:27 AM

ok, thanks. We will have a look.

dtretyakov wrote Mar 31, 2013 at 9:21 AM

I've found that it previous pull request breaks few unit tests, so in the next pull request it was fixed.
http://aspnetwebstack.codeplex.com/SourceControl/network/forks/dtretyakov/Issue952/contribution/4378

Please check it by integration tests.

kenegozi wrote May 16, 2013 at 8:53 PM

dtretyakov - thanks a million for spotting the issue, reporting, and coming up with a solution.

please take a look at the fix we ended up doing. Very similar in essence to your proposed solution, however we also have made some improvements to the design of the code, as well as added a test that have failed with StackOverflow on the previous code and succeed now.

dtretyakov wrote May 17, 2013 at 5:53 AM

Ken,

Your fix of this issue looks really great and much more cleaner than mine.

Just one question: is it necessary to use Async suffixes in the TAP-based methods in the aspnetwebstack code conventions? My question concerns WriteSegment method in the MimeBodyPart class. For instance, here http://msdn.microsoft.com/en-us/library/hh873175.aspx recommended to use it.

Also, I can see additional small code improvements in the WriteSegment method. Actually it's not necessary to use Task awaiter in this method, because you are not using any exception handlers here. I propose following refactoring:
public Task WriteSegmentAsync(ArraySegment<byte> segment)
{
    var stream = GetOutputStream();
    return stream.WriteAsync(segment.Array, segment.Offset, segment.Count);
}
What do you think about this?

Thanks,
Dmitry.