Skip to content

Commit

Permalink
deps: drop deprecated RulesDefinitionXmlLoader
Browse files Browse the repository at this point in the history 
- RulesDefinitionXmlLoader is deprecated and will be dropped in SQ 10
- RulesDefinitionXmlLoader does not populate the OWASP categories and
can't be extended (see spotbugs#392)
- Loading/unloading the plugins dynamically from jar files leads to file
leaks because SpotBugs does not close the classloaders (solves #128)
- The Groovy scripts seem to need some fixes to work with Groovy 4
  • Loading branch information
@gtoison
gtoison committed May 14, 2022
1 parent f533ff9 commit 20eeed3
Show file tree
Hide file tree
Showing 28 changed files with 888 additions and 391 deletions.
41 changes: 7 additions & 34 deletions pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,19 +84,6 @@
</exclusions>
</dependency>


<dependency>
<groupId>org.sonarsource.sslr-squid-bridge</groupId>
<artifactId>sslr-squid-bridge</artifactId>
<version>2.7.0.377</version>
<exclusions>
<exclusion>
<groupId>org.codehaus.sonar</groupId>
<artifactId>sonar-plugin-api</artifactId>
</exclusion>
</exclusions>
</dependency>

<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
Expand All @@ -114,14 +101,12 @@
<groupId>com.mebigfatguy.sb-contrib</groupId>
<artifactId>sb-contrib</artifactId>
<version>${sbcontrib.version}</version>
<scope>provided</scope>
</dependency>

<dependency>
<groupId>com.h3xstream.findsecbugs</groupId>
<artifactId>findsecbugs-plugin</artifactId>
<version>${findsecbugs.version}</version>
<scope>provided</scope>
</dependency>

<dependency>
Expand Down Expand Up @@ -189,6 +174,13 @@
<scope>test</scope>
</dependency>

<dependency>
<groupId>org.picocontainer</groupId>
<artifactId>picocontainer</artifactId>
<version>2.15</version>
<scope>test</scope>
</dependency>

<dependency>
<groupId>org.sonarsource.orchestrator</groupId>
<artifactId>sonar-orchestrator</artifactId>
Expand Down Expand Up @@ -359,25 +351,6 @@
</executions>
</plugin>


<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-dependency-plugin</artifactId>
<executions>
<execution>
<id>copy-dependencies</id>
<phase>process-resources</phase>
<goals>
<goal>copy-dependencies</goal>
</goals>
<configuration>
<stripVersion>true</stripVersion>
<outputDirectory>${project.build.outputDirectory}</outputDirectory>
<includeArtifactIds>annotations,jsr305,sb-contrib,findsecbugs-plugin</includeArtifactIds>
</configuration>
</execution>
</executions>
</plugin>
<plugin>
<groupId>org.sonarsource.sonar-packaging-maven-plugin</groupId>
<artifactId>sonar-packaging-maven-plugin</artifactId>
Expand Down
76 changes: 1 addition & 75 deletions src/main/java/org/sonar/plugins/findbugs/FindbugsConfiguration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import edu.umd.cs.findbugs.Project;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.io.StringWriter;
import java.io.Writer;
import java.nio.charset.StandardCharsets;
Expand All @@ -46,7 +45,6 @@
import org.apache.commons.io.FileUtils;
import org.apache.commons.lang.StringUtils;
import org.sonar.api.PropertyType;
import org.sonar.api.Startable;
import org.sonar.api.batch.ScannerSide;
import org.sonar.api.batch.fs.FilePredicates;
import org.sonar.api.batch.fs.FileSystem;
Expand All @@ -71,7 +69,7 @@
import static java.lang.String.format;

@ScannerSide
public class FindbugsConfiguration implements Startable {
public class FindbugsConfiguration {

private static final Logger LOG = Loggers.get(FindbugsConfiguration.class);
private static final Pattern JSP_FILE_NAME_PATTERN = Pattern.compile(".*_jsp[\\$0-9]*\\.class");
Expand Down Expand Up @@ -125,12 +123,6 @@ public void initializeFindbugsProject(Project findbugsProject) throws IOExceptio
}
}

copyLibs();
if (annotationsLib != null) {
// Findbugs dependencies are packaged by Maven. They are not available during execution of unit tests.
findbugsProject.addAuxClasspathEntry(annotationsLib.getCanonicalPath());
findbugsProject.addAuxClasspathEntry(jsr305Lib.getCanonicalPath());
}
findbugsProject.setCurrentWorkingDirectory(fileSystem.workDir());
}

Expand Down Expand Up @@ -370,72 +362,6 @@ public boolean isAllowUncompiledCode() {
return config.getBoolean(FindbugsConstants.ALLOW_UNCOMPILED_CODE).orElse(FindbugsConstants.ALLOW_UNCOMPILED_CODE_VALUE);
}

private File jsr305Lib;
private File annotationsLib;
private File fbContrib;
private File findSecBugs;

public void copyLibs() {
if (jsr305Lib == null) {
jsr305Lib = copyLib("/jsr305.jar");
}
if (annotationsLib == null) {
annotationsLib = copyLib("/annotations.jar");
}
if (fbContrib == null) {
fbContrib = copyLib("/sb-contrib.jar");
}
if (findSecBugs == null) {
findSecBugs = copyLib("/findsecbugs-plugin.jar");
}
}

@Override
public void start() {
// do nothing
}

/**
* Invoked by PicoContainer to remove temporary files.
*/
@SuppressWarnings("RV_RETURN_VALUE_IGNORED_BAD_PRACTICE")
@Override
public void stop() {
if (jsr305Lib != null) {
jsr305Lib.delete();
}
if (annotationsLib != null) {
annotationsLib.delete();
}
if (fbContrib != null) {
fbContrib.delete();
}

if (findSecBugs != null) {
findSecBugs.delete();
}
}

private File copyLib(String name) {
try (InputStream input = getClass().getResourceAsStream(name)) {
File dir = new File(fileSystem.workDir(), "findbugs");
FileUtils.forceMkdir(dir);
File target = new File(dir, name);
FileUtils.copyInputStreamToFile(input, target);
return target;
} catch (IOException e) {
throw new IllegalStateException("Fail to extract Findbugs dependency", e);
}
}

public File getFbContribJar() {
return fbContrib;
}

public File getFindSecBugsJar() {
return findSecBugs;
}

public static List<PropertyDefinition> getPropertyDefinitions() {
String subCategory = "FindBugs";
return ImmutableList.of(
Expand Down
90 changes: 49 additions & 41 deletions src/main/java/org/sonar/plugins/findbugs/FindbugsExecutor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,11 @@
*/
package org.sonar.plugins.findbugs;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Throwables;
import com.google.common.collect.Lists;
import edu.umd.cs.findbugs.BugCollection;
import edu.umd.cs.findbugs.BugInstance;
import edu.umd.cs.findbugs.BugPattern;
import edu.umd.cs.findbugs.DetectorFactory;
import edu.umd.cs.findbugs.DetectorFactoryCollection;
import edu.umd.cs.findbugs.FindBugs;
import edu.umd.cs.findbugs.FindBugs2;
Expand All @@ -41,13 +41,17 @@
import org.slf4j.LoggerFactory;
import org.sonar.api.batch.ScannerSide;
import org.sonar.api.batch.fs.FileSystem;
import org.sonar.api.batch.rule.ActiveRules;
import org.sonar.api.config.Configuration;
import org.sonar.api.rule.RuleKey;
import org.sonar.plugins.findbugs.rules.FindbugsRules;

import java.io.File;
import java.io.FileReader;
import java.io.IOException;
import java.io.OutputStream;
import java.io.PrintStream;
import java.net.URI;
import java.net.URISyntaxException;
import java.net.URL;
import java.util.*;
Expand Down Expand Up @@ -89,17 +93,8 @@ public FindbugsExecutor(FindbugsConfiguration configuration, FileSystem fs, Conf
this.fs = fs;
this.config = config;
}

@VisibleForTesting
Collection<ReportedBug> execute() {
return execute(true);
}

public Collection<ReportedBug> execute(boolean useAllPlugin) {
return execute(useAllPlugin,useAllPlugin);
}

public Collection<ReportedBug> execute(boolean useFbContrib, boolean useFindSecBugs) {

public Collection<ReportedBug> execute(ActiveRules activeRules) {
// We keep a handle on the current security manager because FB plays with it and we need to restore it before shutting down the executor
// service
SecurityManager currentSecurityManager = System.getSecurityManager();
Expand All @@ -111,7 +106,6 @@ public Collection<ReportedBug> execute(boolean useFbContrib, boolean useFindSecB
Locale.setDefault(Locale.ENGLISH);

OutputStream xmlOutput = null;
Collection<Plugin> customPlugins = null;
ExecutorService executorService = Executors.newSingleThreadExecutor();
try (FindBugs2 engine = new FindBugs2(); Project project = new Project()) {
configuration.initializeFindbugsProject(project);
Expand All @@ -121,8 +115,8 @@ public Collection<ReportedBug> execute(boolean useFbContrib, boolean useFindSecB
return new ArrayList<>();
}

customPlugins = loadFindbugsPlugins(useFbContrib,useFindSecBugs);

loadFindbugsPlugins();
disableUnnecessaryDetectors(project.getConfiguration(), activeRules);
disableUpdateChecksOnEveryPlugin();

engine.setProject(project);
Expand Down Expand Up @@ -189,7 +183,6 @@ public Collection<ReportedBug> execute(boolean useFbContrib, boolean useFindSecB
} finally {
// we set back the original security manager BEFORE shutting down the executor service, otherwise there's a problem with Java 5
System.setSecurityManager(currentSecurityManager);
resetCustomPluginList(customPlugins);
executorService.shutdown();
IOUtils.closeQuietly(xmlOutput);
Thread.currentThread().setContextClassLoader(initialClassLoader);
Expand Down Expand Up @@ -242,39 +235,32 @@ public Object call() {
}
}

private Collection<Plugin> loadFindbugsPlugins(boolean useFbContrib,boolean useFindSecBugs) {
ClassLoader contextClassLoader = Thread.currentThread().getContextClassLoader();
public static Map<String, Plugin> loadFindbugsPlugins() {
ClassLoader contextClassLoader = FindbugsExecutor.class.getClassLoader();

List<String> pluginJarPathList = Lists.newArrayList();
List<String> pluginJarPathList = new ArrayList<>();
try {
Enumeration<URL> urls = contextClassLoader.getResources("findbugs.xml");
while (urls.hasMoreElements()) {
URL url = urls.nextElement();
pluginJarPathList.add(normalizeUrl(url));
}
//Add fb-contrib plugin.
if (useFbContrib && configuration.getFbContribJar() != null) {
// fb-contrib plugin is packaged by Maven. It is not available during execution of unit tests.
pluginJarPathList.add(configuration.getFbContribJar().getAbsolutePath());
}
//Add find-sec-bugs plugin. (same as fb-contrib)
if (useFindSecBugs && configuration.getFindSecBugsJar() != null) {
pluginJarPathList.add(configuration.getFindSecBugsJar().getAbsolutePath());
}
} catch (IOException e) {
throw new IllegalStateException(e);
} catch (URISyntaxException e) {
throw new IllegalStateException(e);
}
List<Plugin> customPluginList = Lists.newArrayList();
Map<String, Plugin> plugins = new HashMap<>();

for (String path : pluginJarPathList) {
try {
Plugin plugin = Plugin.addCustomPlugin(new File(path).toURI(), contextClassLoader);
if (plugin != null) {
customPluginList.add(plugin);
LOG.info("Loading findbugs plugin: " + path);
}
URI uri = new File(path).toURI();
Plugin plugin = Plugin.getAllPluginsMap().get(uri);
if (plugin == null) {
LOG.info("Loading findbugs plugin: " + path);
plugin = Plugin.addCustomPlugin(uri, contextClassLoader);
}
plugins.put(plugin.getPluginId(), plugin);
} catch (PluginException e) {
LOG.warn("Failed to load plugin for custom detector: " + path);
LOG.debug("Cause of failure", e);
Expand All @@ -287,7 +273,7 @@ private Collection<Plugin> loadFindbugsPlugins(boolean useFbContrib,boolean useF
}
}

return customPluginList;
return plugins;
}

private static String normalizeUrl(URL url) throws URISyntaxException {
Expand All @@ -303,12 +289,34 @@ private static void disableUpdateChecksOnEveryPlugin() {
}
}

private static void resetCustomPluginList(Collection<Plugin> customPlugins) {
if (customPlugins != null) {
for (Plugin plugin : customPlugins) {
Plugin.removeCustomPlugin(plugin);
public static void disableUnnecessaryDetectors(UserPreferences userPreferences, ActiveRules activeRules) {
for (DetectorFactory detectorFactory : DetectorFactoryCollection.instance().getFactories()) {
boolean enabled = !detectorFactory.isReportingDetector() || detectorFactoryHasActiveRules(detectorFactory, activeRules);

userPreferences.enableDetector(detectorFactory, enabled);
}
}

private static boolean detectorFactoryHasActiveRules(DetectorFactory detectorFactory, ActiveRules activeRules) {
Collection<String> repositories = FindbugsRules.repositoriesForPlugin(detectorFactory.getPlugin());

if (repositories.isEmpty()) {
LOG.warn("Detector {} is activated because it is not from a built-in plugin, cannot check if there are some active rules", detectorFactory);
return true;
}

for (BugPattern bugPattern : detectorFactory.getReportedBugPatterns()) {
String bugPatternType = bugPattern.getType();

for (String repository : repositories) {
RuleKey ruleKey = RuleKey.of(repository, bugPatternType);

if (activeRules.find(ruleKey) != null) {
return true;
}
}
}

return false;
}

}
Loading

0 comments on commit 20eeed3

Please sign in to comment.