Should a library throw exceptions that reveal its internal call stack?

Question

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;
    }
}

Answer 1

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.

Answer 2

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...

Answer 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.