2

While researching this question, I've found a lot of sources like this:

Basically, it should be deemed a criminal offense to use "throw ex".

The experts I'm seeing this from seem to be working from the assumption that exceptions should provide as much information as possible. But is that really true? If a library that's not open source contains private function calls that then get included in a stack trace, isn't including that information in an exception just confusing at best and insecure at worst? The information I've found so far doesn't seem to address that point.

Let's say I have some C# code like this:

static void Main(string[] args)
{
    TestException();
}

public static void TestException()
{
    NewMethod();
}

private static void NewMethod()
{
    throw new NotImplementedException();
}

This will generate a stack trace like this:

   at ConsoleTest.Program.NewMethod()
   at ConsoleTest.Program.TestException()
   at ConsoleTest.Program.Main(String[] args)

Is that really desired, or is it better practice to have a library throw exceptions that reveal only the publicly exposed method that was called, like this?

public static void TestException()
{
    try
    {
        NewMethod();
    }
    catch (Exception ex)
    {
        throw ex;
    }
}
6
  • 8
    C# can be decompiled quite easily, so I'm not sure what privacy you're preserving by blowing the stack trace. Dec 19, 2019 at 0:32
  • 1
    @RobertHarvey - Perhaps, but there's still the question of whether it's good practice to include methods in a stack trace that aren't part of a library's public interface and the library user would have no reason to recognize Dec 19, 2019 at 0:43
  • 3
    "Good practice" depends on what your goals are. If your goals are well-meaning but ineffective secrecy (i.e. security theater), then by all means blow the stack trace. But if there might be some value in digging a complete stack trace out of a log to help you troubleshoot some obscure, random bug, then the only hope of that happening is to either preserve the stack trace or capture it in an inner exception. Dec 19, 2019 at 0:45
  • 1
    If your library's private methods divides by zero because my input got mangled in some some funky math don't try to hide it from me. I'm all for detail free layers of abstraction but when things break show me the strata. If I have to file a bug against your private method I will. Dec 19, 2019 at 0:49
  • 4
    Exceptions, especially in "code I paid for", should tell your caller how to fix the problem pretty directly. That normally means catching low level exceptions and throwing new ones in a manner that tells the caller what they did wrong against the interface. If an error occurs that you did not expect, it's probably most helpful to wrap it in an exception that informs the caller to contact your support team with inner_exception or whatever...
    – svidgen
    Dec 19, 2019 at 1:28

3 Answers 3

6

First, you should understand that (as Robert H. intelligently points out), c# is easily decompiled. So hiding the stack trace in a library when the binaries area publicly available does not really add much privacy or security.

That being said, there are some cases where it makes sense to hide the details of the inner workings so as to avoid confusion and to keep things loosely coupled. For example, take this simple lookup:

static Dictionary<string, string> _users = null;

static void InitializeUsers()
{
    _users = GetUsersFromDatabase();
}

static string GetUser(string id)
{
    return _users[id.Trim()];
}

In this case, a programmer who wishes to call GetUser() would need to catch a KeyNotFoundException to detect the condition where the user was not found. That couples the caller to the implementation. It might make better sense to replace that exception with an ArgumentException:

static string GetUser(string id)
{
    try
    {
        return _users[id.Trim()];
    }
    catch (KeyNotFoundException exception)
    {
        throw new ArgumentException("User ID is not valid");
    }
}

This keeps things tidy and communicates proper semantic meaning of the exception being thrown.

But you have to be careful or you can create problems and frustration. Imagine, for example, that you also decide to catch the case where the caller has provided a null id, which would cause a null reference exception when calling Trim(). You might be tempted to write this:

static string GetUser(string id)
{
    try
    {
        return _users[id.Trim()];
    }
    catch (Exception exception) when (exception is KeyNotFoundException || exception is NullReferenceException)
    {
        throw new ArgumentException("User ID is not valid");
    }
}

Do you see the problem?

What if there was some sort of failure calling InitializeUsers, so the dictionary was never initialized? That would also raise a null reference exception. And you have taken away the ability for the programmer to tell the difference. They might spend hours wondering why their perfectly good user ID is being treated as a null and pull their hair out.

For that reason, unless you are absolutely sure you have covered all the bases, it is very helpful to include the original stack trace:

catch (Exception exception) when (exception is KeyNotFoundException || exception is NullReferenceException)
{
    throw new ArgumentException("User ID is not valid", exception);
}

That way if you accidentally introduced an unexpected exception, such as initialization failure, then you give the developer the tools they need to isolate the problem.

1
  • This is a good answer, thank you Dec 23, 2019 at 16:39
4

I think you have the right idea but picked the wrong security boundary.

Developers are presumed to be local administrators, can launch a process under the debugger, have access to the physical binaries, can decompile or disassemble. I can set a breakpoint in NewMethod(). Actually most of these operations don't require elevation.

The corollary is to not put anything of value into a client-side binary.

A better security boundary to consider is a network. I would avoid disclosing a call stack remotely or on an API. Because here the binaries aren't available for inspection and it can get callers thinking about how to exploit that null reference exception...

1
  • This is a good answer, thank you. I too was thinking that a library can't be decompiled so easily if it's only accessed remotely. Dec 23, 2019 at 16:42
3

If you write an closed-source library, you presumably sell it to a client. With that, you also presumably sell a support contract. Or at least expect to receive support requests.

Now which kind of bug report do you want to get from your client? The one where the full stack trace of your library is preserved, or the one where you only know the top-level function that was called?

Stack traces are always useful to someone. You should not destroy them.

0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.