Audit Thrift RPC handlers to identify mismatched TException types

[ctubbsii]

The problem:

• Generated Thrift interfaces always add TException as an allowed exception to every method, in addition to any user-defined Thrift exception types.
• User-defined Thrift exception types extend TException, so they do not result in a compile error if thrown from handler code that implements the interface.
• The user-defined Thrift exception types that are declared in the IDL (aka, the ones that are supposed to be thrown from user code) have different handling behavior than ones that are not expected to be thrown; the expected ones get returned to the client and can be handled with catch blocks; the unexpected ones cause a TApplicationException to be returned instead.

This combination of facts leads to the possibility of having mismatched exceptions thrown from RPC handler code that is not properly handled by calling client code.

For example, consider the IDL:

i64 doTask(1:string name) throws (1:BadTaskNameException ex)
string waitForTask(1:i64 taskId) throws (1:InvalidTaskException ex)

Since the generated code includes TException, and since both of the exceptions above are TExceptions, one could write the following valid Java code:

@Override
public long doTask(String name) throws BadTaskNameException, TException {
  if (!name.startsWith("task")) {
    throw new InvalidTaskException(); // this shouldn't be thrown, but it is allowed because it is a TException
  }
  // ... do more work here ...
}

Just as bad, the throws TException could be replaced with a more narrow sub-type of TException, and say throws InvalidTaskException. The compiler would allow this, because you can always narrow the exception type in an implementing class or subclass, but this isn't any more correct, because it won't have correct behavior. The only allowed user-exception for the doTask method is BadTaskNameException. No other TExceptions should be thrown, because they will not be returned to the client, and will instead result in a TApplicationException.

This problem definitely exists in Accumulo code in at least one place, caused by #3177 and an attempted fix in #6048.

What to do

All Accumulo RPC handler methods (that is, any non-generated method that implements a Thrift-generated Iface interface) should be audited to ensure that they:

1. Do NOT declare throws TException, because that allows invalid exception types to fall through unnoticed.
2. Do NOT declare throws for any TException subclass that is not defined as a thrown exception type in the corresponding *.thrift IDL, because that results in a mismatch between what was declared as allowed and what is being thrown, with incorrect behavior.
3. (Optional) Contain all the exceptions declared in the IDL (even those unused), to ensure documentation of which ones are allowed. It's generally okay to throw fewer exceptions than those declared on the interface, but it might be better to keep them in for the RPC methods if other QA tooling doesn't complain, just as documentation.

Any existing violation of these needs to be rewritten to wrap the exception in one of the declared allowable exception types or, if TApplicationException is acceptable for that scenario, be wrapped in an appropriate subclass of RuntimeException.

If we can find some way to write a unit test to automate these checks, that might be very useful. The test may be able to search through a list of handlers, then look at the exceptions declared on the interface, exclude the TException, and fail if the implementing method declares a thrown exception that is not in that set.

Related: For the code changes in #3177, several related methods were changed to throw a new ThriftPropertyException type. However, not all similar methods were modified. Some namespace and system methods were unchanged. These methods should also be audited to ensure that they are behaving correctly. Even if they don't violate the RPC exception checks listed above, they may not be written to handle the exceptional property validation checks in a way that is consistent with their related methods.