Handle exceptions in JarInfer

[navahgar]

In the following code, JarInfer analysis concludes that the parameter o should be nonnull, which is incorrect.

private String paramCanBeNull(Object o) {
   try {
     return o.toString();
   } catch (NullPointerException e) {
     return "";
   }
}

In general, JarInfer does not seem to handle exceptions. This is because JarInfer currently uses ExceptionPrunedCfg for its analysis.

[shubhamugare]

I looked at the WALA ShrikeCFG implementation that we are using.

ShrikeCFG::computeEdges for blocks which have the last instruction as a potentially excepting instruction adds exceptional edges to every corresponding catch block and also to the exit block (in the cases when none of the catch blocks catches all the exceptions). For a basic block outside the try block, it adds an edge to the exit block because the enclosing method says it can throw an exception.

When we convert this CFG to prunedCFG we remove all the exceptional edges and unreachable nodes in the graph after removing edges. prunedCFG does not have enough information about this. But, just CFG has too many edges, which are not of our interest.

[lazaroclapp]

That makes sense from a soundness perspective, of course: every method called might thrown an unchecked exception that could need to be propagated out of that method. On the other hand, those extra edges mean basically nothing dominates the exit block (since all calls just have an exceptional edge pointing to the exit, right?). That's only going to become more of a problem as we try to make our analysis more precise.

I wonder if the best we can do long-term is to special case try/catch blocks that look for NullPointerException or any of its super classes and just not take into account argument dereferences if they happen inside the try block of such construct. We would then need to handle the cases where the catch block itself performs a null dereference or re-throws the exception, though... mmm...

[shubhamugare]

If we are not gaining much information from the try block which can potentially throw a caught NPE. We can remove that block from the bytecode in the pre-processing step?

[lazaroclapp]

If we are not gaining much information from the try block which can potentially throw a caught NPE. We can remove that block from the bytecode in the pre-processing step?

You mean, in the case where we have try {... } catch(NPE_TYPE e) { ... } we just omit that entire part of the AST?

I can think of two reasons not to do that:

• We still want to analyze return statements in that code
• We still might need to know under which circumstances the code in the catch block might trigger, and the @NonNull requirement might still be there, depending on what the catch block does.

In the case above, the correct answer is @Nullable Object o, but consider:

private String paramCantBeNull(Object o) {
   try {
     return o.toString();
   } catch (NullPointerException e) {
     logError(e);
     throw e;
   }
}

Or even:

private String paramCantBeNull(SomeT o) {
   try {
     return o.foo().toString();
   } catch (NullPointerException e) {
     return o.bar().toString();
   }
}

Either way o can't be null there, even if o.foo() can, so we need to mark @NonNull SomeT o.

[shubhamugare]

That will make it weaker. But safer than current implementation? Unless we can do something better which handles all the possible cases.

[navahgar]

I think keeping the current implementation with <ExceptionPrunedCFG> and adding a special case for the code in this issue (while carefully handling the cases Lazaro pointed out in https://github.com/uber/NullAway/issues/318#issuecomment-500042156) is good enough for now.