A code-quality improving contest!
This is about what I found in EPiServer and why it concerns *you*, and a contest for you to enter in. Not only for the glory, but for the fine prizes!
My wife is very understanding, she let’s me ramble on about my everyday nerdy problems and says nothing if I pull a half-nighter (I don’t do full-nighters any more, the privilege of age aka experience) digging really deep into that memory dump and not even replying when spoken to.
The other day, I received an e-mail from a Swedish on-line forum with the message being essentially: “We’ve been hacked and our password storage is not safe. You should change your password on other sites if it’s also used here”. I went off in a spin, telling her about the simple fact that we’ve known for 40 years how to store passwords securely and irreversibly!
She then says: ‘You’re scaring me. You always go on about simple things like that and how the same mistakes keep being made over and over. It sounds like an industry full of programmers who don’t know their profession.’
I could not really say anything to that. The evidence is that it’s true. Then again I guess it’s human to err. But still… It is scary.
Shameless plug: If you want a tool to simply your life when a sites’ password database gets hacked, or if you just want help with managing all your passwords check out Xecrets on-line password generator and store.
Now to the real topic of this post and another even simpler rule than how to store passwords.
Do not ever catch unspecified exceptions silently.
That’s not a very hard rule to follow. But it’s getting violated every day, hundreds if not thousand times even as you read this.
On a site that I am involved in we started getting reports about intermittent behavior concerning logins.
I asked for logs, then increased logging levels, and more logs. (This was greatly simplified by the consolidated logs produced by the remote logging service.)
After about 4 hours of pouring over the logs, my own code and reflecting on the EPiServer code – I found the following:
public override MembershipUser GetUser(object providerUserKey, bool userIsOnline)
{
foreach (MembershipProvider provider in this.ActiveMembershipProviders)
{
try
{
MembershipUser user = provider.GetUser(providerUserKey, userIsOnline);
if (user != null)
{
this.CurrentMembershipUser = user;
this.CurrentProvider = provider;
return user;
}
continue;
}
catch
{
continue;
}
}
return null;
}
This causes *any* exception to return as-if the user was not found! This in turn causes rather unpleasant effects in our case, since the calling code now assumes that the user does not exist. And of course it turns out we can get a SQL Server deadlock here!
But the real point is not that I found a bug in EPiServer. My code has bugs too. Lots of them. But… I should hope there are no silent catch-alls!
Everyone now, do a ‘find in files’ in Visual Studio with something like the following regular expression on your code base:
(catch~(:b*\())|(catch:b*\(:b*Exception.*\))
Then, check each and every one and in *all* cases add at least logging! Do not *ever* eat an exception silently.
There is never any reason to consume an exception silently. Never. Let’s repeat that. Never. Ok, just to make things clear: Never. Got it? Never…
Unfortunately this has been said so many times before, and still…
So, let’s have a contest!
Use the above regular expression or whatever tools you have to find and remove silent consuming of unspecified exceptions. The one who posts the most found and fixed will get a computer related book of their choice (within reason) from me personally. The contest lasts for 30 days, and requires at least 3 entrants and > 10 fixes total for the price to be sent. There’s no shame in fixing bugs! I’m going to do the same on my own code base, who knows…
Oh, and I’ll buy a coffee mug for anyone who can provide a convincing argument for a single situation where it’s appropriate to silently consume an unspecified exception.
PS – The contest is open for EPiServer too!
Awesome idea, I'll give it a try and see whether I catch any in my code. If I do find any, hopefully they'll be the exception rather than the rule :)
Ha! Here goes:
Convincing argument 1:
Eh.... urm... oh... nah - forget it!
You're absolutely right. Never swallow exceptions silently! Ever!
Here is another one. Do not add try catch blocks like this:
try {
SomeCodeHere();
TypicallyInvolvingADatabase();
CouldBeAnythingReally();
} catch (Exception e)
{
// Look ma, the code standard says to catch exceptions
// and I'm doing it! I must be good!
throw new Exception(e.Message);
}
I won't offer books or coffe mugs, only fame to the person that explains why. Svante, you cannot attend :-)
Steve - why can't I? Pretty please? Can I at least add the, possibly more subtle, followup question:
Why and how is 'throw e;' different from 'throw;' in the sample above, and which should you typically use?
Also - I did as promised and ran my regexp on my own code base for my Xecrets service, and to my delight I did not do any silent catch-all. Some third party code that I have included in source form did though...
Youre catching *all* exceptions, but rethrows them as the "anonymous" Exception.
For example, if you catch a "WebException", you'd rethrow it as a simple "Exception", thus loosing lots of useful information.
Actually, I think that allowing you to do a "throw new Exception()" is probably one of the biggest designflaws in .Net - Exception should've been abstract!
/johan
Ok, Svante, you can pitch in too, if you can add to oJohans excellent answer :-)
Gee wiz! Here goes...
Johan O is absolutely correct, but I'd like to add that the most specific useful information that is lost is the stack trace!
This is also the case in the more subtle (and very common) mistake of thinking that 'throw e;' is rethrowing. It's not. It's throwing an exception object from the new location. The fact that the exception instance happens to come from a caught exception rather than being instantiated in the catch-handler makes no difference. The stack trace is lost once again!
So - rethrowing is *not* 'throw e;'. It's 'throw;'. That's what you should typically do after logging or similar action if you really decided you had to catch 'System.Exception'. Don't do 'throw e;'.
Finally, there's the case where you've caught an exception, but would like to throw a different exception to the caller. Be sure to use the constructor for your Exception that includes the Innner Exception where you pass the original exception. That's where those 'Inner Exception' stack traces come frome that we've all seen on yellow background from our ASP.NET sites.
This is getting off-topic, and becoming a forum-discussion on Exceptions.
So - anyone dared to run that simple regexp on their code and report the result? Come on - we're professionals are we not? We care about what we do, and that it's correct and proper craftsmanship, right? You wouldn't be happy if your plumber ignored all the standards and professional best practices about how to make water-tight seals in your bathroom would you? You'd sue them and/or make them come back and do it right. And a plumber have only half the hourly rate/salary we typically do!