[fixed] Error parsing HTTP headers (1 Viewer)

mm1352000

Retired Team Member
  • Premium Supporter
  • September 1, 2008
    21,577
    8,224
    Home Country
    New Zealand New Zealand
    I concur - the UPnP thing has nothing to do with firewalls.
    The message comes from here:
    https://github.com/MediaPortal/Medi...tructure/Utils/HTTP/SimpleHTTPMessage.cs#L148

    Basically MP2 expects the message format to be one or more headers of the format:
    [<whitespace]<header name>[<whitespace]:[<whitespace]<header value>[<whitespace]\r\n

    The final header line should be followed by an additional \r\n, and then comes the body.

    The way I read the code, the most likely way this message could be generated is if the stack receives a message with a whitespace header line or without a body.

    Hopefully the first case is obvious. It shouldn't happen... but such is life when programmers are lazy! :)

    In the second case it would be enough for the message to have no body and end with \r\n followed by optional whitespace.

    In both cases it is a completely harmless situation apart from the annoying log entries.

    I'd propose to solve this by:
    Code:
      int index = line.IndexOf(':');
      if (index == -1)
        if (string.IsNullOrEmpty(line))
          continue;
        throw new InvalidDataException("Invalid HTTP header line '{0}'", line);

    ...or...

    Code:
     string header = Encoding.UTF8.GetString(data, 0, numHeaderBytes).Trim();
     string[] lines = header.Split(new string[] {"\r\n"}, StringSplitOptions.None);

    ...or...

    Code:
     string header = Encoding.UTF8.GetString(data, 0, numHeaderBytes);
     string[] lines = header.Split(new string[] {"\r\n"}, StringSplitOptions.RemoveEmptyEntries);

    Basically the goal is to not log unless a non-whitespace line does not contain a colon :)).

    I note that the line "string header = Encoding.UTF8.GetString(data, 0, numHeaderBytes - 4);" is actually a little dangerous. Consider what happens:
    1. In the unlikely case that the message length is less than 4 bytes.
    2. The top while loop is escaped due to running out of bytes.
    First case will cause an exception. Probably this would be caught.
    Second case will lead to loss or modification of header data. Not good.

    mm
     

    mnmr

    Retired Team Member
  • Premium Supporter
  • December 27, 2008
    180
    102
    Copenhagen
    Home Country
    Denmark Denmark
    I'd propose to solve this by...

    Or maybe just by using HttpListener, which would use the HTTP request parsing built-in to .NET ;)

    Although CRLF is the line termination sequence defined by the RFC, the spec does recommend also allowing for just LF (source).

    Code:
      int index = line.IndexOf(':');
      if (index == -1)
        if (string.IsNullOrEmpty(line))
          continue;
        throw new InvalidDataException("Invalid HTTP header line '{0}'", line);

    The throw statement in the above would always execute (unless the forum has magically filtered out the curly braces). It would also be technically wiser to do the NullOrEmpty check before getting the IndexOf ;)

    Code:
    string header = Encoding.UTF8.GetString(data, 0, numHeaderBytes);
    string[] lines = header.Split(new string[] {"\r\n"}, StringSplitOptions.RemoveEmptyEntries);

    This would be my preferred choice among the given options, with the CRLF caveat mentioned above (but I see that the rest of the code is also CRLF-only).

    I note that the line "string header = Encoding.UTF8.GetString(data, 0, numHeaderBytes - 4);" is actually a little dangerous.

    Good catch, doesn't look entirely bulletproof to me either.
     

    MrTechno

    Retired Team Member
  • Premium Supporter
  • February 27, 2011
    1,256
    511
    London
    Home Country
    United Kingdom United Kingdom
    I think HttpListener is too high level - all the code that deals with endpoints and streams is hidden away. The MP2 messages are coming over UDP.

    *edit* and the class is sealed so no chance of using the subclassing and reusing the parser
     
    Last edited:

    mrj

    Portal Pro
    January 27, 2012
    252
    100
    I concur - the UPnP thing has nothing to do with firewalls.
    The message comes from here:
    https://github.com/MediaPortal/Medi...tructure/Utils/HTTP/SimpleHTTPMessage.cs#L148

    Does this actually work ?

    Code:
    internal void ParseHeaderAndBody(Stream stream, out string firstLine)
    {
    const int HEADER_BUF_INC = 2048;
    byte[] data = new byte[HEADER_BUF_INC];
    int numHeaderBytes = 0;
    int b;
    while ((b = stream.ReadByte()) != -1)
    {
    if (numHeaderBytes - 1 == data.Length)
    IncreaseBuffer(data, HEADER_BUF_INC);
    data[numHeaderBytes++] = (byte) b;
    if (numHeaderBytes > 4 && data[numHeaderBytes-3] == '\r' && data[numHeaderBytes-2] == '\n' &&
    data[numHeaderBytes-1] == '\r' && data[numHeaderBytes] == '\n')
    break;
    }
    Can data[numHeaderBytes] == '\n' be true?
    /EDIT
    1 chance in 256?

    /mrj
     
    Last edited:

    MrTechno

    Retired Team Member
  • Premium Supporter
  • February 27, 2011
    1,256
    511
    London
    Home Country
    United Kingdom United Kingdom
    Unit tests anyone?

    *edit* This is not meant to be a criticism, more of a note to self and a lament that I didn't bring a laptop on holiday
     
    Last edited:

    mnmr

    Retired Team Member
  • Premium Supporter
  • December 27, 2008
    180
    102
    Copenhagen
    Home Country
    Denmark Denmark
    I think HttpListener is too high level - all the code that deals with endpoints and streams is hidden away. The MP2 messages are coming over UDP.

    *edit* and the class is sealed so no chance of using the subclassing and reusing the parser

    Fair enough, those are reasonable arguments for building your own.
     

    mrj

    Portal Pro
    January 27, 2012
    252
    100
    Hi

    Does this actually work ?

    Can data[numHeaderBytes] == '\n' be true?
    /EDIT
    1 chance in 256?

    No it does not work.

    Replace
    Code:
    data[numHeaderBytes++] = (byte) b;
    if (numHeaderBytes > 4 && data[numHeaderBytes-3] == '\r' && data[numHeaderBytes-2] == '\n' &&
    data[numHeaderBytes-1] == '\r' && data[numHeaderBytes] == '\n')
    break;

    with

    Code:
    data[numHeaderBytes] = (byte) b;  // Move increment from here
    if (numHeaderBytes > 4 && data[numHeaderBytes-3] == '\r' && data[numHeaderBytes-2] == '\n' &&
    data[numHeaderBytes-1] == '\r' && data[numHeaderBytes] == '\n')
    break;
     
    numHeaderBytes++; // To here

    HTH mrj
     

    morpheus_xx

    Retired Team Member
  • Team MediaPortal
  • March 24, 2007
    12,073
    7,459
    Home Country
    Germany Germany
    I moved out the discussion about http header parsing into new thread to keep better track on it...
     

    morpheus_xx

    Retired Team Member
  • Team MediaPortal
  • March 24, 2007
    12,073
    7,459
    Home Country
    Germany Germany
    Unit tests anyone?
    I actually just wrote one and already found a parsing error (last header was 4 bytes to short). Going to check the different line break styles later today and the rest of parsing logic.
     

    Users who are viewing this thread

    Top Bottom