Spot the bug: stupid .Net Framework bug

I came across a very silly (and annoying) bug in the HttpWebResponse.CharacterSet property. Let's see if you can spot it (code extracted with reflector):

public string get_CharacterSet()
{
  this.CheckDisposed();
  string text1 = this.m_HttpResponseHeaders.ContentType;
  if ((this.m_CharacterSet == null) && !ValidationHelper.IsBlankString(text1))
  {
    this.m_CharacterSet = string.Empty;
    string text2 = text1.ToLower(CultureInfo.InvariantCulture);
    if (text2.Trim().StartsWith("text/"))
    {
      this.m_CharacterSet = "ISO-8859-1";
    }
    int num1 = text2.IndexOf(";");
    if (num1 > 0)
    {
      while ((num1 = text2.IndexOf("charset", num1)) >= 0)
      {
        num1 += 7;
        if ((text2[num1 - 8] == ';') || (text2[num1 - 8] == ' '))
        {
          while ((num1 < text2.Length) && (text2[num1] == ' '))
          {
            num1++;
          }
          if ((num1 < (text2.Length - 1)) && (text2[num1] == '='))
          {
            num1++;
            int num2 = text2.IndexOf(';', num1);
            if (num2 > num1)
            {
              this.m_CharacterSet = text1.Substring(num1, num2).Trim();
              break;
            }
            this.m_CharacterSet = text1.Substring(num1).Trim();
            break;
          }
        }
      }
    }
  }
  return this.m_CharacterSet;
}

I won't give you any hints as to what it is, but quite frankly I'm amazed that this bug didn't get caught. Don't these people write unit tests?

Yes, this bug is in .Net Framework 2.0 RTM.

Ok... I'll bite.  I've never used HttpWebResponse.CharacterSet, but it seems that the line:

this.m_CharacterSet = text1.Substring(num1, num2).Trim();

should read:

this.m_CharacterSet = text1.Substring(num1, num2-num1).Trim();

So, what's my prize?

How has nobody noticed that before?

bondsbw wrote:

Ok... I'll bite.  I've never used HttpWebResponse.CharacterSet, but it seems that the line:

this.m_CharacterSet = text1.Substring(num1, num2).Trim();

should read:

this.m_CharacterSet = text1.Substring(num1, num2-num1).Trim();

Yep, you got it. Which means that if the Content-Type header contains an additional ";" after the charset definition, calling CharacterSet will fail with an ArgumentOutOfRangeException.

99% of websites don't put anything after the charset, that's why this has probably gone unnoticed. In fact, the only site that does this that I know of (and that was in fact the site where I encountered this bug) is groups.msn.com.

Test while you're coding...
Test when you're done coding...
Test when you're recoding...
When you're done, test some more...

On a related note, groups.msn.com links to an image
http://mir.atdmt.com/b/MSMSNMATCMOC/V05_120X90_LoveJustNotForFeb_matP_.gif

which returns this header:
Content-Type: image/gif;charset=ISO-8859-1

Maurits wrote:

Test while you're coding...
Test when you're done coding...
Test when you're recoding...
When you're done, test some more...

Bad advise... It isn't about "when to test;" in that you could follow your advise to the letter and not find the bug. It is about *HOW* to test.

Manip wrote:

Bad advise... It isn't about "when to test;" in that you could follow your advise to the letter and not find the bug. It is about *HOW* to test.

Touché.  If your testing strategy doesn't catch bugs, it's just a waste of time.

It looks like this doesn't handle attributes in quotes either...

Content-Type: text/html; charset="iso-8859-1";

I'm surprised there's not a general header-parsing function for

header: foo; bar="baz"

Maurits wrote:

It looks like this doesn't handle attributes in quotes either...

Content-Type: text/html; charset="iso-8859-1";

I'm surprised there's not a general header-parsing function for

header: foo; bar="baz"

The .Net framework should ONLY support HTTP-standard syntax, not stuff you just made up to break the procedure.

Manip wrote:

Maurits wrote:It looks like this doesn't handle attributes in quotes either... Content-Type: text/html; charset="iso-8859-1"; I'm surprised there's not a general header-parsing function for header: foo; bar="baz"

The .Net framework should ONLY support HTTP-standard syntax, not stuff you just made up to break the procedure.

You are in a snit, aren't you...

Oh, and *ahem*

3.6 Transfer Codings

       parameter               = attribute "=" value
       attribute               = token
       value                   = token | quoted-string

3.7 Media Types
Parameters MAY follow the type/subtype in the form of attribute/value pairs (as defined in section 3.6).

(my emphasis in red)

It is content-coding, not Transfer Codings.

Interesting... it seems a trailing semicolon is NOT correct, at least according to the RFC:

media-type     = type "/" subtype *( ";" parameter )

So groups.msn.com's header is not RFC-compliant.

EDIT: doesn't make it any less of a bug, of course.
EDIT2: Manip, the Content-Type header definition is in section 3.7.  It refers back to the parameter definition in 3.6.
EDIT3: Uh-oh... mea culpa time.

Section 3.4 limits charset parameters to tokens:

	charset = token

I was wrong, Manip is right.  Sorry.