The solution for False Positive of java try-catch

[Wang Wenlin]

Currently, the False Positive of java -try-catch is a common issue. Like this:

InputStream is = null;
try {
  is = new InputStream("no_such_file");
} catch (Exception e) {
}
try {
  if (is != null) {
    is.close();
  }
} catch (Exception e) {
}

The above code snippet will be report as "InputStream" should be close.

The root cause is in "org/sonar/java/cfg/CFG.java" line 790 in function buildTryStatement().

for (Block catchBlock : catches) {
  currentBlock.addSuccessor(catchBlock);
}

Here, the catch block was being added as a branch of try block. So, in CFG, the above code snippet is:

               InputStream is = null;
                    /             \
                  /                 \
             is=new ...      // Empty catch block
                  \                 /
                    \             /
               // Empty block
                     /       \
                   /           \
 if (is != null) ...     // Empty catch block
                \               /
                  \           /
                 function exit...

So, it is not so hard to understand the False Positive now.

How to resolve this False Positive? It is quite hard because CFG want to simulate try-catch via branch mechanism. However, we can use a some simple, tricky solution to decrease those kind of False Positive.

1. go through the all blocks inside try-block, and found out the first possible exception occupation point.

2. break this block before(or after) the exception occupation point according to the expression type. (normal expression - after, new-object expression - before)

3. add the catch-block as a branch of the remains of this block.

So, the CFG of the above code snippet will be:

                   InputStream is = null;
                          /       \
                        /          \
             is=new ...     // Empty catch block
                      \              /
                       \           /
                   // Empty block
                            |  
                            |
                    if (is != null) ...
                        /       \
                       /         \
                      \        // Empty catch block
                        \          /
                          \      /
                      function exit ...

So, the False Positive is GONE!!!

The attachment is my patch. And, I think if we find out all the possible exception occupation point, the result should be more accurate. But the number of branches may be too many.

 

[Nicolas Peru]

Hi, 

Thanks for your interest and the patch. In order to ease the review would you mind opening a pull request on the github project rather than submitting a diff file by email ?

Thanks, I'll try to review it within the next weeks.

Cheers, 

--
You received this message because you are subscribed to the Google Groups "SonarQube" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/e0f23fda-de68-4a3a-ae4b-0f32f6417c88%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--

Nicolas PERU | SonarSource

Senior Developer
http://sonarsource.com

https://twitter.com/benzonico

[Nicolas Peru]

Hi,

Thanks for your contribution,

 just having a better look at your email : basically, you are trying to solve this ticket : https://jira.sonarsource.com/browse/SONARJAVA-1422 

I just want to draw your attention to the fact that finding the  first possible exception occupation point as you name it is not so trivial in the end as, almost every expression in java can raise an exception so your approximation  of this might work in this case but this has to be tested extensively to ensure no raise of other false positive in other cases (nested expressions, etc...  ). 

I'll schedule the mentioned ticket for next version of java plugin and linked in to this thread for a first resolution. 

Thanks again.

Cheers, 

[Wenlin Wang]

Hi,

I rewrite my solution as https://github.com/SonarSource/sonar-java/pull/870

I think this is the RIGHT way to resolve any try-catch-finally problem totally!

Please have a look!

Thanks.

--

Best regards,
Wenlin Wang

[Nicolas Peru]

Hi, 

Thanks for the contribution ! 

I took the time to review it carefully, and while I tend to agree with the approach I have several concern on the solution. 

Let me explain in more details : 

Your approach is, if I'm correct, tolist all the exceptions caught by a try statement and always keep track of the closest enclosing try statement (if any). 

Then, during walking of the exploded graph you are enqueuing node based on the exception that can be thrown by method invocation (this denomination includes call to constructors here). 

Apart from some implementation details that I have concern with (nullable lists for instance) I am bit more perplex about the why this is done in the ExplodedGraph Walker (EGW) : as far as I understand, nothing prevents to actually solve the paths already in the CFG making the separation of concerns a bit clearer. 

This will also avoid to store the currently raised exceptions in the program state and will simplify the implementation. 

I also have some worries about the quality of results that we are going to get from this  (performance wise and amount of false negative/positive (ie: runtime exceptions will be completely overlooked by this approach resulting in false negative compared to previous approach) ) but I tend to think that it is worth exploring more in details. 

As a consequence of all this, I will now work on this approach but rather at CFG level directly and not in the EGW as the path can be implemented directly there.

Tell me if you think I missed something. 

Thanks again for the contribution and suggestion. 

Cheers,