C#: What’s wrong with this code #5 – Discussion


Thanks for all the comments.

As a refresher, here’s the code that we were looking at:

enum ResponseType { ReturnValues, InvalidInput }

string CreateWebText(string userInput, ResponseType operation) {
   switch (operation) {
      case ResponseType.ReturnValues:
         userInput = “<h1>Values</h1>” + FilterOutBadStuff(userInput);
         break;
      case ResponseType.InvalidInput:
         userInput = “<h1>Invalid</h1>” + FilterOutBadStuff(userInput);
         break;
   }
   return userInput;
}

First, there were a few comments about not generating HTML yourself, not echoing input back to the user, and not writing something that is already present in ASP.NET.

Agreed, agreed, agreed. There are lots of attacks that depend on this, and you shouldn’t even do it for debugging.

There was considerable discussion about whether the variable “userInput” should be reused. In most cases, I think that it’s best to come up with a new name, but there are cases where it makes more sense to reuse the name. I don’t feel strongly about this case.

Finally, on to the big thing – or what I define as the big thing, at least – the handling of the enum.

Enums in C# are double-duty – they serve both as sets and as bit fields. That means that an enum can have any value that is valid on the underlying type. In this case, there’s no type defined, so it’s an int. In other words, you can write:

ResponseType responseType = (ResponseType) 157;

and things will work just fine.

So, we need a way to validate the value (assuming, of course, that we’re going to keep this routine. We wouldn’t in practice, but humor me…)

Here’s a modification that does that:

string CreateWebText(string userInput, ResponseType operation) {
   if (!Enum.IsDefined(typeof(ResponseType), operation)
      throw new InvalidArgumentException(…);

   switch (operation) {
      case ResponseType.ReturnValues:
         userInput = “<h1>Values</h1>” + FilterOutBadStuff(userInput);
         break;
      case ResponseType.InvalidInput:
         userInput = “<h1>Invalid</h1>” + FilterOutBadStuff(userInput);
         break;
   }
   return userInput;
}

And that works just fine.

Or does it?

 

Comments (19)

  1. Nicholas Allen says:

    Enum.IsDefined fails when someone modifies the enumeration to add a new case and you fall out of the switch statement without hitting a default handler. It’s also an extraordinarily expensive operation.

  2. Len Weaver says:

    Adding:

    default:

    throw new InvalidArgumentException(…);

    …to the switch would be better for a number of reasons. First, at some point in the future you may add additional values to the enum. Enum.IsDefined would return true if you use one of those new values even if you forget to update your switch to handle them. Also, as Nicholas has mentioned, Enum.IsDeinfed in a dog.

  3. Luc Cluitmans says:

    My first response when I see a ‘switch()’ over some (non-Flags) enum type is to ensure that there is a ‘default’ branch that throws an exception. And this response has allowed me to detect several potential bugs in a very early stage in the past already…

    As the previous responders have already explained, that prevents several problems. And, as mentioned, Enum.IsDefined is an expensive operation (reflection based, I presume).

    From a language design point of view, if I would have designed the C# ‘switch’ statement, I would have seriously considered *requiring* a ‘default’ branch in every ‘switch’. Leaving it out is rarely a good idea in my experience. Is there an FxCop rule already that checks that?

  4. CesarGon says:

    And if I had designed enums in C#, I would have made the compiler issue a warning at:

    ResponseType responseType = (ResponseType) 157;

    or even an error. I would also do run-time checks and throw an exception if an out-of-range integer is casted into an enum.

    I have a post on this in my blog:

    http://spaces.msn.com/members/cesargon2/Blog/cns!1pGomXRO1m3e0JP8HmKF05Ag!122.entry

    CesarGon.

  5. AT says:

    Enum.IsDefined does not work for flags. JFYI 😉

    As for performance costs – it’s not so big – access of hashtable with Type key and then binary search in array of values.

    P.S> InvalidArgumentException is Java or custom Microsoft-internal NDA protected codename for ArgumentOutOfRangeException 😉

  6. Ian Cooper says:

    Not, I think, the point but switching on type is usually a ‘bad smell’ that suggests we should be looking at polymorphism to solve our problem.

    That aside I would favor the addition of a default: statement block over explicit checking for the enum type being defined.

  7. Ian Cooper says:

    Not, I think, the point but switching on type is usually a ‘bad smell’ that suggests we should be looking at polymorphism to solve our problem.

    That aside I would favor the addition of a default: statement block over explicit checking for the enum type being defined.

  8. Thomas Eyde says:

    Replace

    ResponseType responseType = (ResponseType) 157;

    with

    ResponseType responseType = (ResponseType) invalidResponse;

    and the compiler has no chance to know if it’s valid or not.

  9. Eric Sowell says:

    I’ve also found it useful to throw exceptions in a default case statement when you have not yet implemented functionality for that enum value.

    But I wouldn’t recommend making a default required. Let’s say you have an enum with 10 values, and you want to in a particular piece of code execute some extra bit of code in the case of 5 of those. Having to add a default in such a case would not be beneficial if nothing needed to be done, because you would have to create an empty default statement. I don’t see the value in that.

    Of course, maybe there’s a better way to handle this outside of a switch. I’m just not sure it would be beneficial for the compiler to require it.

  10. Alastair Dallas says:

    The revised code, without the default: that many have suggested, returns the un-filtered userInput. This invites abuse, but of course you wouldn’t skip the default, and you wouldn’t re-use the variable name, and you wouldn’t duplicate existing code, so it’s a bit moot.

  11. Darren Oakey says:

    " My first response when I see a ‘switch()’ over some (non-Flags) enum type is to ensure that there is a ‘default’"

    —–

    Let’s change that: How about "my first response when I see a switch() over some enum type is to think ‘what idiot coded this’ and remove it from the system"

  12. Joe White says:

    Eric Sowell: If your case statement only handles 5 of the 10 enum values, then your code has problems, both with brittleness and with readability.

    At the very least, you should have a "default:" block containing a comment, where you explain *why* you don’t handle the other cases. But even that is asking for trouble down the road, because at some point you’ll add a new value to the enum that *should* be handled in this switch, and you’ll never realize you messed up until your customers start complaining.

    What you *should* be doing is:

    switch (myEnum) {

    case MyEnum.Value1:

    DoSomething1();

    break;

    case MyEnum.Value2:

    DoSomething2();

    break;

    case MyEnum.Value3:

    DoSomething3();

    break;

    case MyEnum.Value4:

    DoSomething4();

    break;

    case MyEnum.Value5:

    DoSomething5();

    break;

    case MyEnum.Value6:

    case MyEnum.Value7:

    case MyEnum.Value8:

    case MyEnum.Value9:

    case MyEnum.Value10:

    break; // no action needed here

    default:

    throw new ArgumentOutOfRangeException(…);

    }

    That way, when you add a new value to the enum but forget to add it here, you’ll find out sooner (via an exception) rather than later (via subtly-wrong behavior).

  13. Eric Sowell says:

    I actually like that idea quite a lot. Thanks Joe.

  14. saw blade says:

    I don’t know the so difficult computer language,i just learn the basic html,and i have built 3 sites all with html:http://www.aaff.net ,http://www.aauu.net ,and http://www.aagg.net .Please check them and tell me what i should to correct.

  15. Dr. Pangloss says:

    In addition to following Joe’s recommendation above, another source of potential trouble is that FilterOutBadStuff is called in each branch of the enum, and could be missed in the case of a bad operation. The following line should be added above the switch statement:

    userInput = FilterOutBadStuff(userInput);

  16. Brett Alcorn says:

    Yuk… let’s just stop using enums and switches and write something which won’t allow invalid types, eg:

    public abstract class ResponseType

    {

    public string CreateWebText(string userInput)

    {

    return "<h1>" + HeaderText + "</h1>" + FilterOutBadStuff(userInput);

    }

    protected abstract string HeaderText

    {

    get;

    }

    }

    public class ReturnValueResponseType : ResponseType

    {

    protected override string HeaderText

    {

    get

    {

    return "Values";

    }

    }

    }

    public class InvalidInputResponseType : ResponseType

    {

    protected override string HeaderText

    {

    get

    {

    return "Invalid";

    }

    }

    }

    … etc

  17. andychu says:

    http://www.safetytech.cn“>http://www.safetytech.cn

    http://www.safetytech.cn“>http://www.safetytech.cn/dcfs.asp

    http://www.zcfounder.com“>http://www.zcfounder.com/yinxiebing”>http://www.zcfounder.com“>http://www.zcfounder.com/yinxiebing

    http://www.zcfounder.com“>http://www.zcfounder.com/piyan”>http://www.zcfounder.com“>http://www.zcfounder.com/piyan

    http://www.zcfounder.com“>http://www.zcfounder.com/gongshangzhuce”>http://www.zcfounder.com“>http://www.zcfounder.com/gongshangzhuce

    http://www.zcfounder.com“>http://www.zcfounder.com/gongsizhuce”>http://www.zcfounder.com“>http://www.zcfounder.com/gongsizhuce

    http://www.zcfounder.com“>http://www.zcfounder.com/zhucegongsi”>http://www.zcfounder.com“>http://www.zcfounder.com/zhucegongsi

    http://www.zcfounder.com“>http://www.zcfounder.com/office/”>http://www.zcfounder.com“>http://www.zcfounder.com/office/

    http://g0gle.51.net/office

    http://g0gle.51.net/gongshangzhuce

    http://g0gle.51.net/gongsizhuce

    http://g0gle.51.net/zhucegongsi

    http://g0gle.51.net/biaozhisheji

    http://g0gle.51.net/dailijizhang

    http://g0gle.51.net/dazhejipiao

    http://g0gle.51.net/gongzuofu

    http://g0gle.51.net/jianfei

    http://g0gle.51.net/niupixuan

    http://g0gle.51.net/pifubing

    http://g0gle.51.net/piyan

    http://g0gle.51.net/polycom

    http://g0gle.51.net/shipinhuiyi

    http://g0gle.51.net/yinxiebing

    http://www.jinxique.com“>http://www.jinxique.com

    http://www.bestel.com.cn“>http://www.bestel.com.cn

    http://www.bestel.com.cn“>http://www.bestel.com.cn/polycom

    http://www.110fat.com“>http://www.110fat.com“>http://www.110fat.com“>http://www.110fat.com“>http://www.110fat.com“>http://www.110fat.com“>http://www.110fat.com“>http://www.110fat.com

    http://www.zcfounder.com“>http://www.zcfounder.com/patent/fanyico2/fanyi_15.htm”>http://www.zcfounder.com“>http://www.zcfounder.com/patent/fanyico2/fanyi_15.htm

    http://andy197819.51.net/xgwz

    http://www.lrxdq.com/”>http://www.lrxdq.com/

    http://www.rituo.com“>http://www.rituo.com/knowledge”>http://www.rituo.com“>http://www.rituo.com/knowledge

    http://wpromotion.51.net/xgwz

    http://www.zcfounder.com“>http://www.zcfounder.com/rzcping”>http://www.zcfounder.com“>http://www.zcfounder.com/rzcping

    http://www.zcfounder.com“>http://www.zcfounder.com/yujia”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia“>http://www.zcfounder.com“>http://www.zcfounder.com/yujia”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia/fanyico2/fanyi_15.htm”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia“>http://www.zcfounder.com“>http://www.zcfounder.com/yujia”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia/fanyico2/fanyi_15.htm“>http://www.zcfounder.com“>http://www.zcfounder.com/yujia”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia“>http://www.zcfounder.com“>http://www.zcfounder.com/yujia”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia/fanyico2/fanyi_15.htm”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia“>http://www.zcfounder.com“>http://www.zcfounder.com/yujia”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia/fanyico2/fanyi_15.htm

    http://www.zcfounder.com“>http://www.zcfounder.com/yujia”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia“>http://www.zcfounder.com“>http://www.zcfounder.com/yujia”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia/fanyico2/fanyi_13.htm”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia“>http://www.zcfounder.com“>http://www.zcfounder.com/yujia”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia/fanyico2/fanyi_13.htm

    http://www.rituo.com“>http://www.rituo.com

    http://www.cebooks.net“>http://www.cebooks.net

    http://www.110fat.com“>http://www.110fat.com“>http://www.110fat.com“>http://www.110fat.com“>http://www.110fat.com“>http://www.110fat.com“>http://www.110fat.com“>http://www.110fat.com

    http://andy197819.51.net/jianfei/“>http://andy197819.51.net/jianfei/

    http://wpromotion.51.net/jianfei/“>http://wpromotion.51.net/jianfei/

    http://www.lrxdq.com

    http://www.ceo863.com

    http://www.zcfounder.com“>http://www.zcfounder.com/yujia”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia“>http://www.zcfounder.com“>http://www.zcfounder.com/yujia”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia/fanyico2/fanyi_15.htm”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia“>http://www.zcfounder.com“>http://www.zcfounder.com/yujia”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia/fanyico2/fanyi_15.htm“>http://www.zcfounder.com“>http://www.zcfounder.com/yujia”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia“>http://www.zcfounder.com“>http://www.zcfounder.com/yujia”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia/fanyico2/fanyi_15.htm”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia“>http://www.zcfounder.com“>http://www.zcfounder.com/yujia”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia/fanyico2/fanyi_15.htm

    http://www.zcfounder.com“>http://www.zcfounder.com/yujia”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia“>http://www.zcfounder.com“>http://www.zcfounder.com/yujia”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia

    http://www.zcfounder.com“>http://www.zcfounder.com/yujia”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia“>http://www.zcfounder.com“>http://www.zcfounder.com/yujia”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia

    http://www.zcfounder.com“>http://www.zcfounder.com google

    http://www.zcfounder.com“>http://www.zcfounder.com/promotion.htm google

    http://www.zcfounder.com“>http://www.zcfounder.com/website.htm

    http://www.zcfounder.com“>http://www.zcfounder.com/oasoft.htm

    http://www.zcfounder.com“>http://www.zcfounder.com/khgl.htm

    http://www.zcfounder.com“>http://www.zcfounder.com/jxc.htm

    http://www.zcfounder.com“>http://www.zcfounder.com/hotel.htm

    http://www.zcfounder.com“>http://www.zcfounder.com/bregis/

    http://www.zcfounder.com“>http://www.zcfounder.com/caigang/

    http://www.zcfounder.com“>http://www.zcfounder.com/cping/

    http://www.zcfounder.com“>http://www.zcfounder.com/design

    http://www.zcfounder.com“>http://www.zcfounder.com/fangwei/

    http://www.zcfounder.com“>http://www.zcfounder.com/giftco/

    http://www.zcfounder.com“>http://www.zcfounder.com/glasses/

    http://www.zcfounder.com“>http://www.zcfounder.com/gps/

    http://www.zcfounder.com“>http://www.zcfounder.com/jiayimin/

    http://www.zcfounder.com“>http://www.zcfounder.com/kouqang/

    http://www.zcfounder.com“>http://www.zcfounder.com/law/

    http://www.zcfounder.com“>http://www.zcfounder.com/ledscreen/

    http://www.zcfounder.com“>http://www.zcfounder.com/lxym/

    http://www.zcfounder.com“>http://www.zcfounder.com/patent/

    http://www.zcfounder.com“>http://www.zcfounder.com/rentcar

    http://www.zcfounder.com“>http://www.zcfounder.com/ticket/

    http://www.zcfounder.com“>http://www.zcfounder.com/transcompany/

    http://www.zcfounder.com“>http://www.zcfounder.com/vod

    http://www.zcfounder.com“>http://www.zcfounder.com/yiminnew/

    http://www.zcfounder.com“>http://www.zcfounder.com/yujia”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia“>http://www.zcfounder.com“>http://www.zcfounder.com/yujia”>http://www.zcfounder.com“>http://www.zcfounder.com/yujia/

    http://www.zcfounder.com“>http://www.zcfounder.com/zhanlan/

    http://www.zcfounder.com“>http://www.zcfounder.com/yuanlin

    http://www.zcfounder.com“>http://www.zcfounder.com/office/”>http://www.zcfounder.com“>http://www.zcfounder.com/office/

    http://www.zcfounder.com“>http://www.zcfounder.com/office1/

    http://www.zcfounder.com“>http://www.zcfounder.com/qichepeijian

    http://www.zcfounder.com“>http://www.zcfounder.com/dianzibaiban

    http://www.zcfounder.com“>http://www.zcfounder.com/zichanpinggu

    http://www.zcfounder.com“>http://www.zcfounder.com/yikatong

    http://www.zcfounder.com“>http://www.zcfounder.com/guangduanji

    http://www.zcfounder.com“>http://www.zcfounder.com/minjin

    http://www.zcfounder.com“>http://www.zcfounder.com/jiankong

    http://www.zcfounder.com“>http://www.zcfounder.com/qianjinding

    http://www.zcfounder.com“>http://www.zcfounder.com/xcdesign

    http://www.zcfounder.com“>http://www.zcfounder.com/fenci

    http://www.zcfounder.com“>http://www.zcfounder.com/cuochuang

    http://www.zcfounder.com“>http://www.zcfounder.com/qchdou

    http://www.zcfounder.com“>http://www.zcfounder.com/mshpx

    http://www.zcfounder.com“>http://www.zcfounder.com/mgjx

    http://www.zcfounder.com“>http://www.zcfounder.com/xfqc

    http://www.zcfounder.com“>http://www.zcfounder.com/shuichuli

    +++++++++++++++++++++++++++++++++++++++++++++

    http://www.zcfounder.com“>http://www.zcfounder.com/craft

    http://www.zcfounder.com“>http://www.zcfounder.com/jieneng

    http://www.zcfounder.com“>http://www.zcfounder.com/syyq

    http://www.zcfounder.com“>http://www.zcfounder.com/fxyq

    http://www.zcfounder.com“>http://www.zcfounder.com/baolingqiu

    http://www.zcfounder.com“>http://www.zcfounder.com/chuchen

    http://www.zcfounder.com“>http://www.zcfounder.com/golf

    http://www.zcfounder.com“>http://www.zcfounder.com/club

    http://www.zcfounder.com“>http://www.zcfounder.com/zhusuji

    http://www.zcfounder.com“>http://www.zcfounder.com/chuimoji

    http://www.zcfounder.com“>http://www.zcfounder.com/bianpingyou

    http://www.zcfounder.com“>http://www.zcfounder.com/hhban

    http: