Common programming mistakes in .Net when handling exceptions? [closed]

Question

What are some of the most common mistakes you've seen made when handling exceptions?

I can think of lots.

First read my article on categorization of exceptions into vexing, boneheaded, fatal and exogenous:

http://ericlippert.com/2008/09/10/vexing-exceptions/

Some common errors:

• Failure to handle exogenous exceptions.
• Failure to handle vexing exceptions.
• Construction of methods that throw vexing exceptions.
• Handling exceptions that you actually cannot handle, like fatal exceptions.

• Handling exceptions that hide bugs in your code; don't handle a boneheaded exception, fix the bug so that it isn't thrown in the first place

• Security error: failure to the unsafe mode

  try
  {
    result = CheckPassword();
    if (result == BadPassword) throw BadPasswordException();
  }
  catch(BadPasswordException ex) { ReportError(ex); return; }
  catch(Exception ex) { LogException(ex); }
  AccessUserData();
  

  See what happened? We failed to the unsafe mode. If CheckPassword threw NetworkDriverIsAllMessedUpException then we caught it, logged it, and accessed the user's data regardless of whether the password was correct. Fail to the safe mode; when you get any exception, assume the worst.

• Security error: production of exceptions which leak sensitive information, directly or indirectly.

  This isn't exactly about handling exceptions in your code, it's about producing exceptions which are handled by hostile code.

  Funny story. Before .NET 1.0 shipped to customers we found a bug where it was possible to call a method that threw the exception "the assembly which called this method does not have permission to determine the name of file C:\foo.txt". Great. Thanks for letting me know. What is stopping said assembly from catching the exception and interrogating its message to get the file name? Nothing. We fixed that before we shipped.

  That's a direct problem. An indirect problem would be a problem I implemented in LoadPicture, in VBScript. It gave a different error message depending upon whether the incorrect argument is a directory, a file that isn't a picture, or a file that doesn't exist. Which means you could use it as a very slow disk browser! By trying a whole bunch of different things you could gradually build up a picture of what files and directories were on someone's hard disk. Exceptions should be designed so that if they are handled by untrustworthy code, that code learns none of the user's private information from whatever they did to cause the exception. (LoadPicture now gives much less helpful error messages.)

• Security and resource management error: Handlers which do not clean up resources are resource leaks waiting to happen. Resource leaks can be used as denial-of-service attacks by hostile partial trust code which deliberately creates exceptions-producing situations.

• Robustness error: Handlers must assume that program state is messed up unless handling a specific exogenous exception. This is particularly true of finally blocks. When you're handling an unexpected exception, it is entirely possible and even likely that something is deeply messed up in your program. You have no idea if any of your subsystems are working, and if they are, whether calling them will make the situation better or worse. Concentrate on logging the error and saving user data if possible and shut down as cleanly as you can. Assume that nothing works right.

• Security error: temporary global state mutations that have security impacts need to be undone before any code that might be hostile can run. Hostile code can run before finally blocks run! See my article on this for details:

http://blogs.msdn.com/ericlippert/archive/2004/09/01/224064.aspx

Re-throwing exceptions like this:

try 
{ 
   // some code here
}
catch(Exception ex)
{
   // logging, etc
   throw ex;
}

This kills the stack trace, making is far less usable. The correct way to rethrow would be like this:

try 
{ 
   // some code here
}
catch(Exception ex)
{
   // logging, etc
   throw;
}

Catching all exceptions when in many cases you should attempt to catch specific exceptions:

try {
  // Do something.
} catch (Exception exc) {
  // Do something.
}

Rather than:

try {
  // Do something.
} catch (IOException exc) {
  // Do something.
}

Exceptions should be ordered from most specific to least.

Rethrowing an exception with a meaningless message.

try
{
    ...
}
catch (Exception ex)
{
   throw new Exception("An error ocurred when saving database changes").
}

You won't believe how often I see code like this running in production.

Nobody is talking about seeing empty catch blocks like these....

 try{  
      //do something
    }
catch(SQLException sqex){  
        // do nothing  
    }

Also never use Exception handling for creating alternate method flows...

 try{  
     //do something  

 }catch(SQLException sqex){  

     //do something else  
 }