[fixed] Error parsing HTTP headers (1 Viewer)

[breese]

I installed this on my client after I found the server would not start with it... Opps
Is there something I can test this with?
I tried to get WMP on my portable to send a movie... Nope
Client logs appear to be clean

 

[mnmr]

@mnmr can you test the attached binary? I think the "invalid http header" issues should be gone.

I am also seeing a server crash. Logs attached.

 

[morpheus_xx]

Oh, sorry. This version contains the IP binding fix. Will post a full installer later today (or you use the build from "screensaver disconnects server..." bug report thread)

Edit: it's the opposite way around: this binary here doesn't contain the changes for IP binding, that's why it fails...

 

[mrj]

Hi
@morpheus_xx

Please fix this as well
replace

protected static void IncreaseBuffer<T>(T[] buffer, uint increment)
    {
      T[] oldBuffer = buffer;
      buffer = new T[oldBuffer.Length + 2048];
      Array.Copy(oldBuffer, 0, buffer, 0, oldBuffer.Length);
}

with

protected static void IncreaseBuffer<T>(T[] buffer, uint increment)
    {
      T[] oldBuffer = buffer;
      buffer = new T[oldBuffer.Length + increment];
      Array.Copy(oldBuffer, 0, buffer, 0, oldBuffer.Length);
    }

mrj

 

[offbyone]

Since net 4.0 there is Array.Resize:
http://msdn.microsoft.com/de-de/library/bb348051(v=vs.110).aspx

 

[morpheus_xx]

thanks @mrj and @offbyone. I reworked more parts of code:
https://github.com/MediaPortal/MediaPortal-2/commit/3674c5e7ca2de4f4687272b0d2f2c9790d30acea

Now using MemoryStream for body handling and added a content-length check. The unit test was very helpful for this case

 

[mrj]

Hi
This looks strange to me

// Check for correct body length
        int contentLength;
        if (int.TryParse(this["CONTENT-LENGTH"], out contentLength) && contentLength != -1 && contentLength != _bodyBuffer.Length)
          throw new InvalidDataException("Invalid HTTP content length: header {0}, actual body: {1}", contentLength, _bodyBuffer.Length);

should'nt CONTENT-LENGTH be in the header not in the body.

mrj

 

[morpheus_xx]

yes, it's a header attribute and defines the length of the body. If the body would have a different length, this is an error.

 

[mnmr]

thanks @mrj and @offbyone. I reworked more parts of code:
https://github.com/MediaPortal/MediaPortal-2/commit/3674c5e7ca2de4f4687272b0d2f2c9790d30acea

This snippet (line 186-187) seems like it could be the root cause of the log errors I am seeing:

    if (index == -1)
        throw new InvalidDataException("Invalid HTTP header line '{0}'", line);

I'm not sure what the spec says on the matter, but if it doesn't cause other errors to go undetected it might be useful to just ignore empty header lines. I haven't looked at any of the surrounding code, so don't feel like I have insight enough into the code to evaluate it - but it certainly looks like an improvement

 

[mnmr]

The second ToUpperInvariant seems to be rendundant (line 73):

index = index.ToUpperInvariant();
return _headers.ContainsKey(index) ? _headers[index.ToUpperInvariant()] : null;

In fact, both the indexer and the two ContainsHeader and SetHeader methods could be simplified if the dictionary was created like this:

protected IDictionary<string, string> _headers = new Dictionary<string, string>(StringComparer.InvariantCultureIgnoreCase);

Some comments for the following code (lines 147-176):

internal void ParseHeaderAndBody(Stream stream, out string firstLine)
{
   byte[] data = new byte[HEADER_BUF_INC];
   int numHeaderBytes = 0;
   int b;
   int delimiterLength = 3;
   while ((b = stream.ReadByte()) != -1)
   {
      if (numHeaderBytes == data.Length)
         Array.Resize(ref data, data.Length + HEADER_BUF_INC);
      data[numHeaderBytes] = (byte)b;
      // Regular CRLF line ending
      if (numHeaderBytes > 4 && data[numHeaderBytes - 3] == CR && data[numHeaderBytes - 2] == LF && data[numHeaderBytes - 1] == CR && data[numHeaderBytes] == LF)
         break;
      // Single LF line ending
      if (numHeaderBytes > 2 && data[numHeaderBytes - 1] == LF && data[numHeaderBytes] == LF)
      {
         delimiterLength = 1;
         break;
      }
      numHeaderBytes++;
   }
   string header = Encoding.UTF8.GetString(data, 0, numHeaderBytes - delimiterLength);

Shouldn't the default delimiterLength be just 2 (for CR+LF)?

Isn't it performance-unwise to read things byte-by-byte? If it was me, I'd grab as much as possible and fill up the buffer before doing any parsing. Once everything is received, you scan for the header/body delimiter in memory using code like this:

        // Finds the starting index of any of the given matching patterns; the result is returned 
        // as an array containing both index and pattern length.
        int[] FindIndexMultiPattern( byte[] data, params byte[][] patterns )
        {
            var matchCount = new int[ patterns.Length ];
            for( var i = 0; i < data.Length; i++ )
            {
                for( var p = 0; p < patterns.Length; p++ )
                {
                    var count = matchCount[ p ];
                    matchCount[ p ] = data[ i ] == patterns[ p ][ count ] ? count + 1 : 0;
                    if( matchCount[ p ] == patterns[ p ].Length )
                        return new[] {i - count, patterns[ p ].Length};
                }
            }
            return null;
        }

// some usage
const byte CR = 13;
const byte LF = 10;
var separatorPatternCRLF = new [] {CR, LF, CR, LF};
var separatorPatternLF = new [] {LF, LF};
           
byte[] data;
var splitIndex = FindIndexMultiPattern( data, separatorPatternLF, separatorPatternCRLF );
var header = data.Take( splitIndex[ 0 ] ).ToArray();
var body = data.Skip( splitIndex[ 0 ] + splitIndex[ 1 ] ).ToArray();

I could be missing something though, since I haven't really dealt with parsing HTTP requests before