ctubbsii opened a new issue, #6061:
URL: https://github.com/apache/accumulo/issues/6061
## 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 `TException`s, one could write the following valid Java
code:
```java
@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 `TException`s 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
