Bug 21717 - System.ServiceModel.Channel.WebMessageEncoder exceptions causing WCF service to crash
Summary: System.ServiceModel.Channel.WebMessageEncoder exceptions causing WCF service ...
Status: CONFIRMED
Alias: None
Product: Class Libraries
Classification: Mono
Component: WCF assemblies (show other bugs)
Version: unspecified
Hardware: All All
: --- normal
Target Milestone: Future Release
Assignee: Bugzilla
URL:
Depends on:
Blocks:
 
Reported: 2014-07-30 18:00 UTC by metalize
Modified: 2017-10-12 13:44 UTC (History)
4 users (show)

See Also:
Tags:
Is this bug a regression?: ---
Last known good build:


Attachments
Test program to cause the described bug. (602 bytes, application/octet-stream)
2014-07-30 18:00 UTC, metalize
Details

Description metalize 2014-07-30 18:00:47 UTC
Created attachment 7540 [details]
Test program to cause the described bug.

Description of Problem:
System.ServiceModel.Channel.WebMessageEncoder throws exceptions when either the HTTP Post Content-Type header is missing or Content-Type is not a valid Content-Type.

Steps to reproduce the problem:
1. For the Content-Type ArgumentNullException bug run the attached code and then also run:
curl http://127.0.0.1:12345/test -d "data" -H "Content-Type:"
2. For the Content-Type FormatException bug run the attached code and then also run:
 curl http://127.0.0.1:12345/test -d "blah" -H "Content-Type: invalid"

Actual Results:
An uncaught exception from WCF. The WCF service stops running.

Expected Results:
Not an uncaught exception from WCF. The WCF service should continue running.

How often does this happen? 
Every POST without a Content-Type or with an invalid Content-Type.

Additional Information:
Current mono code:
public override bool IsContentTypeSupported (string contentType)
{
    if (contentType == null)
        throw new ArgumentNullException ("contentType");
    return true; // anything is accepted.
}

A possible fixed version:
public override bool IsContentTypeSupported (string contentType)
{
    if (String.IsNullOrEmpty(contentType))
    {
        //Not supported if it's not a valid string
        return false;
    }

    try
    {
        //Not supported if it's not a valid content type
        new System.Net.Mime.ContentType(contentType);
    }
    catch (FormatException)
    {
        return false;
    }

    //everything else is supported
    return true;
}
Comment 1 Martin Baulig 2014-08-04 16:49:53 UTC
No point in fixing error messages in a class that's not implemented.

Message encoding in WCF is supported yet and large parts of the implementation are missing.
Comment 2 metalize 2014-08-04 16:59:54 UTC
It's not fixing error messages.

The service actually crashes. I'm pretty sure it's not supposed to do that. The .NET implementation does not crash on the same input.

With a small code change similar to the sample code posted, the WCF service won't crash on bad input.
Comment 3 Martin Baulig 2014-08-04 17:24:47 UTC
The problem is simply, large parts of WCF are not implemented.  This class in particular has not seen a single change in the last four years and there are a lot of FIXME's and not implemented parts in it which all come from the initial implementation in 2008.

From looking at the parts that are implemented and what I remember about message encoders in WCF, this class is supposed to convert the message.

I'm not sure what's supposed to happen when the Content-Type: header is missing - probably auto-detect it somehow?  But that doesn't seem to be implemented.

Your suggested change also doesn't look right: it would accept every Content-Type that's "valid" according to the System.Net.Mime API, but that's far more types than this class could actually handle.

And for "Content-Type: invalid" - I would assume we're supposed to crash ... ?
Comment 4 metalize 2014-08-04 17:31:21 UTC
I'm not saying my change is the correct implementation. It's just one way I was able to modify the mono source so that my WCF service will no longer crash if missing or unsupported Content-Type occurs. So, no, I'm not saying my changes are the way to go.

I am saying that the current behavior of the WCF service crashing is undesirable and definitely a bug. Whether or not the full WCF has been implemented or not, the current mono code allows a user with a web browser to crash my WCF service.
Comment 5 Martin Baulig 2014-08-04 17:41:27 UTC
Sure it's better than it was before, but let's not replace one broken thing with another not quite so broken thing, but take the chance of making it at least somewhat right.

So let's first split this into two separate problems:

1.) No Content-Type provided.  If that's crashing your app, then I think we should just pick some reasonable default type - either by removing that null check or doing something else, I don't really care.

2.) Invalid Content-Type - if I see this correctly, then this class only supports a few known types: "application/xml", "text/xml", "application/json", "text/json", "application/octet-stream".  If your app uses something else and that works, then add it.  But instead of using that System.Net.Mime API, let's just whitelist those vew supported types, how does this sound?

If possible, could you please also provide a pull request with these changes; assign to me (baulig on github) and I'll merge it :-)

Thanks,
Martin

Note You need to log in before you can comment on or make changes to this bug.