S2095: Resources should be closed - detect custom close methods

[Andreas Mandel]

Hi,

usually I have a IoUtil / DbUtil . close method, that closes all kind of resources by also checking for null and do exceptionhandling with proper logging - I think this pattern is very common.

It would be helpful if the detector could either follow these method calls and detect these methods automatically by following the call graph or let us configure such methods to be considered as fine.

Kind Regards,
Andreas.

[Nicolas Peru]

Hi Andreas, 

Would you be able to share a code sample to illustrate your case ? 

Thanks.

--
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/a9f8c452-49da-44ab-8889-9988d13a7530%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--

Nicolas PERU | SonarSource

Senior Developer
http://sonarsource.com

https://twitter.com/benzonico

[Andreas Mandel]

Hi Nicolas,

thanks for picking this up. Sure:

package com.example.sonarqube;

import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.io.IOException;

/**
 * S2095 FP demo.
 */
public class CloseResource {
    public void test() throws FileNotFoundException, IOException {
        final FileInputStream is = new FileInputStream("/tmp/foo"); // this stream is flagged as not being closed
        try {
            int in = is.read(); // skip uhl satz
            while (in != -1) {
                in = is.read();
            }
        } finally {
            IoUtil.close(is);
        }
    }
}

with IoUtil:

package com.example.sonarqube;

import java.io.IOException;
import java.io.InputStream;
import java.util.logging.Level;
import java.util.logging.Logger;

public class IoUtil {
    private static final Logger LOGGER = Logger.getLogger(IoUtil.class.getName());

    /**
     * Closes the input stream (safe).
     * @param in the input stream that should be closed.
     */
    public static void close(InputStream in) {
        if (in != null) {
            try {
                in.close();
            } catch (IOException x) {
                LOGGER.log(Level.FINE, "Error while closing InputStream.close()", x);
            }
        }
    }
}

This should box the described situation.

Kind Regards,
Andreas.

[Nicolas Peru]

Hi, 

We are working on detecting correctly those cases by, like you put it : following the call graph. This is still experimental work and restricted to a file in our current experimentation. 

This is what we call cross procedural analysis and do hope to deliver to get rid of those issues. 

In the meantime, there is unfortunately not much you can do to work around this on your side beside marking the issue as false positive. 

Cheers, 

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/9c348708-697e-4319-a8da-f6559c8ae5a3%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Andreas Mandel]

Hi Nicolas,

thanks for looking into this.

I understand that the "cross procedural analysis" is a complicated but promising topic. The "deprecated" detector pmd:CloseResource allowed to define alternative close methods (closeTargets) that should be treated as "close" methods. Might be this is, for the time being a easier and quicker solution to detect this calls and get rid of the false positives?

We filter false positives through the "sonar.issue.ignore.multicriteria" properties which is unfortunately not very fine grain. Actually at the end it would collect all classes that do some resource handling to ignore S2095 and so render S2095 useless. :-(

Kind Regards,
Andreas.

[Nicolas Peru]

Hi, 

As you may (or may not) know, we are not keen on adding rule parameters in order to keep configuration cost of analyzer as low as possible. 

I think a good approximation for this (while waiting for X-procedural analysis (with an X for cross, this sounds so awesome ! ;))  would maybe to _also_ consider that a resource is closed when it is passed as an argument to a close or closeQuietly method. 

I am really looking forward for opinions about this suggestion. 

Cheers, 

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/2029a4e0-7d70-46d4-89ce-04479cf85bc8%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Michel Pawlak]

Hi Nicolas,

I don't get how configuration cost would be increased for people not wanting to refine their configuration. They would still use the standard configuration and would be happy with it (you could even imagine hiding all configurable fields unless the user clicks on an "show advanced options" button in th UI). Advanced users, however, would benefit from parametrizable rules.

From a rule development and maintainability cost for SonarSource I can understand your position, but not from an end-user perspective.

Best regards,

Michel

[Andreas Mandel]

Hello Nicolas,

having a list of potential close methods as a temporarily solution, till the cross procedural analysis is available sounds OK to me, especially since the method names would cover my case. :)

One point to the configuration part. I understand your point, that you want to avoid parametrization were possible. Nevertheless IMHO SQ is not in the position to define the definite rules in static software quality. There is a lot of clear stuff but there are others rules with lot of different valid opinions - also caused by different types of software that is checked. Also there are situations like here, where the code implementing the rule is just not smart enough to catch the whole picture.

You can reduce the need of parametrization by providing more separate rules opposed to combining several similar cases in one rule. This also allows to select the right set of rules and to give the cases different severities which is important in many cases.

Kind Regards,
Andreas.

[Nicolas Peru]

Hi Andreas, 

so regarding what we suggested : https://jira.sonarsource.com/browse/SONARJAVA-1670 

Then considering what you and Michel expressed, let me give you a broader picture :  Rule parameters are evil in many many ways : pain to maintain/test, advanced feature so not available for most users, hard to document, hard to validate etc... 

We also see things that way : by resorting to rule parameters we rely on users to sort things out, not on the product. As product maintainer that would be all too easy to do this all the time and that is also why we tend to fight to not fall down that route that could lead to configuration hell.  (I am pretty sure we could imagine parameters for nearly all 400 rules in the java plugin). 

This is why we usually rely on those as the last  resort and fight hard to provide a better solution that would give a great experience (basically no FP) for _everyone_, advanced users or not. 

"You can reduce the need of parametrization by providing more separate rules opposed to combining several similar cases in one rule. This also allows to select the right set of rules and to give the cases different severities which is important in many cases."

This is _exactly_ what we try to do, this is why we don't raise different severities for the same rule (because this would basically go against the quality profile feature of SonarQube platform) and this is why we also provide this rule template : https://nemo.sonarqube.org/coding_rules#rule_key=squid%3AS3546 that could be used for what you want to achieve here (and deactivate S2095). 

Another issue we face is that rule templates like this are very hard to promote properly (I am ready to bet you didn't know about it ? ;) ) (Maybe we should provide a link to it from S2095 description ?) 

Hope that helps you understand what we're doing.

Cheers, 

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/b873c0b8-0702-4cff-8ed6-18b93af624a0%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Michel Pawlak]

Hi Nicolas,

You state that rule "parameters are evil" and list these disadvantages:

1. pain to maintain / test (for SonarSource, you prefer working on other topics)
2. advanced features not available for most users (uh ? I would rather say available to all, used by few users really needing it, maybe you consider it not worth it)
3. hard to document (not hard, but time consuming and thus costly)
4. hard to validate (not hard, but time consuming and thus costly)

As I wrote in my previous post, all these arguments are understandable from a rule development cost and maintainability cost point of view for SonarSource (and thus investment optimisation point of view), but not from an end-user perspective. So please don't "hide" behind words like "evil", and "configuration hell" and just say that you prefer investing money elsewhere because it will benefit to more people. There is nothing bad about this, it's easy to understand and explain if stated clearly instead of using manipulatory words like "evil" or "configuration hell".  

And what about the advantages you don't list ?

• no need to write a custom rule to adapt a non working rule
  
• no need to have developers knowing how to write a rule
• no need to rebuild, rerelease a custom rules project when users want to modify a rule
• no need to restart server to change a rule, when people use a new library
• no need to maintain custom rules
• no need to wait for features that will be available in X versions

In this context, that it's quite surprising that you state that parameterising rules means relying on users. I could be provocative and state that with no parameterised rules you decide to rely on advanced users to help themselves if they need to adapt a rule ("you want it ? develop and pay for it !") That's fine, but at least make it crystal clear, because right now I have the feeling that you serve us an "it's for your own good" argument, the kind of argument one could give to a 6 years old child, not to professionals who have to deal with complaining end users on a daily basis. 

Last but not least, no matter the quality of your product, when end users spot a limitation which could be fixed if the rule was parametrisable they use this to criticise the product as a whole (especially if the rule raises lots of FPs).

Regards,

Michel

[Nicolas Peru]

Hi Michel, 

Just reread my answer : we try to take rule parameters as the last resort and I explained why we are reluctant to them, this does not mean we forbid completely ourselves from it (we do have some parameters on some rules) but we will most of the time try to find a better alternative.

I tend to summarize your argument on upside by  "rule parameters are a nice alternative to custom rules" which I think is a bit of an exageration on your side maybe :) And if you feel pain with custom rules (which would be totally legit), please let us know about it, as this can fairly be improved. 

Cheers, 

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/d5422c8c-0bd8-4c41-9689-68ac8266c8c5%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Michel Pawlak]

Hi,

I tend to summarize your argument on upside by "rule parameters are a nice alternative to custom rules"

No, my point is that we should not have to write a custom rule to adapt an existing platform-provided rule that raises false positives. Not being able to tune a rule is not a good enough reason to write a custom rules. These are great in order to add new controls that are specific to a context that would not benefit the community as a whole, but they are a pain to maintain and they impact rule profiles maintainability when used to replace existing rules (I want to use the Sonar Way profile, but want to replace rule X with my custom rule X' and want to be true each time I update the Sonar Way profile). Parameterisation however is a good alternative in this context and is available right now.

If you consider parameterisation a pain and want an architectural solution, think about an adapter system, where each rule would be adaptable, would be able to retrieve available adapters that can be applied to it (based on its key, version min, version max) when an analysis is started. Each adapter would register itself for a given rule or set of rules (based on rule key and version) and would be able to modify or extend the rule's behaviour (add a behaviour to a chain of behaviours, limit added behaviour to a subset of resources based on preconditions, disable the standard behaviour based on a precondition checked each time the rule is executed, etc). Such adapters being simple POJOs it would be extremely easy for the community to write and UT test them (without having to use the existing test harness). 

The community could then contribute adapter sets as new SQ plugins (for instance a set for OpenJPA, another one for Spring, etc) and adapter sets could be enabled automatically based on Maven dependencies, properties, metadata (jar manifest files for instance) you name it. This would allow you to remove all framework-specific code from the plugin and keep your rules as simple as possible.

If I exclude automatic detection of whether or not an adapter should be executed. On your side it means adding a new extension type for such adapters, an AdaptableRule class that retrieves and applies corresponding adapters, and an API that allows to retrieve information required to evaluate preconditions and execute or not the adapter. No more maintainability pain on your side, and "parameterisation" available on our side.

Cheers,

Michel

[Nicolas Peru]

Hi Michel, 

"No, my point is that we should not have to write a custom rule to adapt an existing platform-provided rule that raises false positives."

I really could not agree more, I don't think we ever recommended this and therefore I am all ear to hear about your cases so we can improve the rules.

Thanks.

Cheers,

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/979fec8a-a20e-477a-a4ff-d90cbe1518cf%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Andreas Mandel]

Hello Peru,

thanks for the details.

Actually I did not know S3546 - a link might help :). Actually we now have the 2 extreme positions - no configuration at all possible and you need to configure all yourselves. Might be sensitive defaults that match S2095 for S3546 would make it even better.

There are other places where one new Squid rule replaces several rules of the old tools with partly different scope (eg. squid:S1166, squid:S2259, squid:S00112, ...) and so make it hard to select the one severity for all the cases.

Kind Regards,

Andreas.

[Nicolas Peru]

Hi, 

For sake of completness :  ticket created to handle this issue : https://jira.sonarsource.com/browse/SONARJAVA-1688

Cheers, 

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/e441f1bb-73ac-4b12-ae85-293a1ba79ac8%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.