Skip to content

Commit

Permalink
spotbugs rule for direct instantiation of BinaryDecoder (#161)
Browse files Browse the repository at this point in the history
  • Loading branch information
radai-rosenblatt authored Jun 15, 2021
1 parent 113b0a3 commit a1fada4
Show file tree
Hide file tree
Showing 18 changed files with 145 additions and 25 deletions.
2 changes: 1 addition & 1 deletion avro-codegen/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ dependencies {
implementation project(":helper:helper")

implementation "commons-io:commons-io:2.6"
implementation "org.apache.logging.log4j:log4j-api:2.11.2"
implementation "org.apache.logging.log4j:log4j-api:2.14.1"
implementation "com.beust:jcommander:1.78"
implementation "org.apache.ant:ant:1.10.7"

Expand Down
4 changes: 2 additions & 2 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ subprojects {
testImplementation "org.testng:testng:6.14.3"

//redirect logging to log4j2 for tests
testRuntimeOnly "org.apache.logging.log4j:log4j-core:2.11.2"
testRuntimeOnly "org.apache.logging.log4j:log4j-slf4j-impl:2.11.2"
testRuntimeOnly "org.apache.logging.log4j:log4j-core:2.14.1"
testRuntimeOnly "org.apache.logging.log4j:log4j-slf4j-impl:2.14.1"
}

test {
Expand Down
4 changes: 2 additions & 2 deletions helper/tests/codegen-110/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ dependencies {
//this block required for resource generation code
implementation project(":helper:tests:helper-tests-common")
//redirect logging to log4j2 for resource generation
resourcegen "org.apache.logging.log4j:log4j-core:2.11.2"
resourcegen "org.apache.logging.log4j:log4j-slf4j-impl:2.11.2"
resourcegen "org.apache.logging.log4j:log4j-core:2.14.1"
resourcegen "org.apache.logging.log4j:log4j-slf4j-impl:2.14.1"
resourcegen files('../codegenClasspath')
}
8 changes: 4 additions & 4 deletions helper/tests/codegen-14/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,8 @@ dependencies {
codegen project(":helper:tests:helper-tests-common")
codegen project(":helper:helper")
//redirect logging to log4j2 for code generation
codegen "org.apache.logging.log4j:log4j-core:2.11.2"
codegen "org.apache.logging.log4j:log4j-slf4j-impl:2.11.2"
codegen "org.apache.logging.log4j:log4j-core:2.14.1"
codegen "org.apache.logging.log4j:log4j-slf4j-impl:2.14.1"
codegen files('../codegenClasspath')

//required because generated code depends on the helper
Expand All @@ -114,7 +114,7 @@ dependencies {
//this block required for resource generation code
implementation project(":helper:tests:helper-tests-common")
//redirect logging to log4j2 for resource generation
resourcegen "org.apache.logging.log4j:log4j-core:2.11.2"
resourcegen "org.apache.logging.log4j:log4j-slf4j-impl:2.11.2"
resourcegen "org.apache.logging.log4j:log4j-core:2.14.1"
resourcegen "org.apache.logging.log4j:log4j-slf4j-impl:2.14.1"
resourcegen files('../codegenClasspath')
}
4 changes: 2 additions & 2 deletions helper/tests/codegen-15/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ dependencies {
//this block required for resource generation code
implementation project(":helper:tests:helper-tests-common")
//redirect logging to log4j2 for resource generation
resourcegen "org.apache.logging.log4j:log4j-core:2.11.2"
resourcegen "org.apache.logging.log4j:log4j-slf4j-impl:2.11.2"
resourcegen "org.apache.logging.log4j:log4j-core:2.14.1"
resourcegen "org.apache.logging.log4j:log4j-slf4j-impl:2.14.1"
resourcegen files('../codegenClasspath')
}
4 changes: 2 additions & 2 deletions helper/tests/codegen-16/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ dependencies {
//this block required for resource generation code
implementation project(":helper:tests:helper-tests-common")
//redirect logging to log4j2 for resource generation
resourcegen "org.apache.logging.log4j:log4j-core:2.11.2"
resourcegen "org.apache.logging.log4j:log4j-slf4j-impl:2.11.2"
resourcegen "org.apache.logging.log4j:log4j-core:2.14.1"
resourcegen "org.apache.logging.log4j:log4j-slf4j-impl:2.14.1"
resourcegen files('../codegenClasspath')
}
4 changes: 2 additions & 2 deletions helper/tests/codegen-17/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ dependencies {
//this block required for resource generation code
implementation project(":helper:tests:helper-tests-common")
//redirect logging to log4j2 for resource generation
resourcegen "org.apache.logging.log4j:log4j-core:2.11.2"
resourcegen "org.apache.logging.log4j:log4j-slf4j-impl:2.11.2"
resourcegen "org.apache.logging.log4j:log4j-core:2.14.1"
resourcegen "org.apache.logging.log4j:log4j-slf4j-impl:2.14.1"
resourcegen files('../codegenClasspath')
}
4 changes: 2 additions & 2 deletions helper/tests/codegen-18/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ dependencies {
//this block required for resource generation code
implementation project(":helper:tests:helper-tests-common")
//redirect logging to log4j2 for resource generation
resourcegen "org.apache.logging.log4j:log4j-core:2.11.2"
resourcegen "org.apache.logging.log4j:log4j-slf4j-impl:2.11.2"
resourcegen "org.apache.logging.log4j:log4j-core:2.14.1"
resourcegen "org.apache.logging.log4j:log4j-slf4j-impl:2.14.1"
resourcegen files('../codegenClasspath')
}
4 changes: 2 additions & 2 deletions helper/tests/codegen-19/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ dependencies {
//this block required for resource generation code
implementation project(":helper:tests:helper-tests-common")
//redirect logging to log4j2 for resource generation
resourcegen "org.apache.logging.log4j:log4j-core:2.11.2"
resourcegen "org.apache.logging.log4j:log4j-slf4j-impl:2.11.2"
resourcegen "org.apache.logging.log4j:log4j-core:2.14.1"
resourcegen "org.apache.logging.log4j:log4j-slf4j-impl:2.14.1"
resourcegen files('../codegenClasspath')
}
7 changes: 6 additions & 1 deletion spotbugs-plugin/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,12 @@ dependencies {

testImplementation project(":helper:helper")
testImplementation "junit:junit:4.13"
testImplementation "com.github.spotbugs:test-harness:4.2.3"
testImplementation ("com.github.spotbugs:test-harness:4.2.3") {
//we dont want the slf4j 1.8-beta this uses - messes up our test logging
exclude group: "org.slf4j"
}
//bring the older slf4j api after excluding the beta above
testImplementation "org.slf4j:slf4j-api:1.7.25"
testImplementation ('org.apache.avro:avro:1.4.1') {
exclude group: "org.mortbay.jetty"
exclude group: "org.apache.velocity"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package com.linkedin.avroutil1.spotbugs;

import edu.umd.cs.findbugs.BugInstance;
import edu.umd.cs.findbugs.BugReporter;
import edu.umd.cs.findbugs.bcel.OpcodeStackDetector;
import org.apache.bcel.Const;

/**
* detects direct instantiations of BinaryDecoder
*/
public class BinaryDecoderInstantiationDetector extends OpcodeStackDetector {
private final BugReporter bugReporter;

public BinaryDecoderInstantiationDetector(BugReporter bugReporter) {
this.bugReporter = bugReporter;
}

@Override
public void sawOpcode(int seen) {
if (seen != Const.NEW) {
return;
}
if (getClassConstantOperand().equals("org/apache/avro/io/BinaryDecoder")) {
// new BinaryEncoder call
BugInstance bug = new BugInstance(this, "BINARY_DECODER_INSTANTIATION", NORMAL_PRIORITY)
.addClassAndMethod(this)
.addSourceLine(this, getPC());
bugReporter.reportBug(bug);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@
/**
* detects direct instantiations of BinaryEncoder
*/
public class BinaryEncoderUsageDetector extends OpcodeStackDetector {
public class BinaryEncoderInstantiationDetector extends OpcodeStackDetector {
private final BugReporter bugReporter;

public BinaryEncoderUsageDetector(BugReporter bugReporter) {
public BinaryEncoderInstantiationDetector(BugReporter bugReporter) {
this.bugReporter = bugReporter;
}

Expand Down
4 changes: 3 additions & 1 deletion spotbugs-plugin/src/main/resources/findbugs.xml
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@
provider="avro-util"
website="http://github.com/linkedin/avro-util">

<Detector class="com.linkedin.avroutil1.spotbugs.BinaryEncoderUsageDetector" reports="BINARY_ENCODER_INSTANTIATION" speed="fast" />
<Detector class="com.linkedin.avroutil1.spotbugs.BinaryEncoderInstantiationDetector" reports="BINARY_ENCODER_INSTANTIATION" speed="fast" />
<Detector class="com.linkedin.avroutil1.spotbugs.BinaryDecoderInstantiationDetector" reports="BINARY_DECODER_INSTANTIATION" speed="fast" />

<BugPattern type="BINARY_ENCODER_INSTANTIATION" category="AVRO" abbrev="BEI"/>
<BugPattern type="BINARY_DECODER_INSTANTIATION" category="AVRO" abbrev="BDI"/>

</FindbugsPlugin>
30 changes: 29 additions & 1 deletion spotbugs-plugin/src/main/resources/messages.xml
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,20 @@
<Details>provides detections related to use of known-problematic avro APIs</Details>
</Plugin>

<Detector class="com.linkedin.avroutil1.spotbugs.BinaryEncoderUsageDetector">
<!-- detector descriptions -->

<Detector class="com.linkedin.avroutil1.spotbugs.BinaryEncoderInstantiationDetector">
<Details>
detects direct instantiations of org.apache.avro.io.BinaryEncoder
</Details>
</Detector>
<Detector class="com.linkedin.avroutil1.spotbugs.BinaryDecoderInstantiationDetector">
<Details>
detects direct instantiations of org.apache.avro.io.BinaryDecoder
</Details>
</Detector>

<!-- bug pattern descriptions -->

<BugPattern type="BINARY_ENCODER_INSTANTIATION">
<ShortDescription>direct BinaryEncoder instantiation</ShortDescription>
Expand All @@ -28,6 +37,25 @@
]]>
</Details>
</BugPattern>
<BugPattern type="BINARY_DECODER_INSTANTIATION">
<ShortDescription>direct BinaryDecoder instantiation</ShortDescription>
<LongDescription>
direct instantiations of org.apache.avro.io.BinaryDecoder, which is not directly accessible in modern avro
</LongDescription>
<Details>
<![CDATA[
<p>
org.apache.avro.io.BinaryDecoder's constructor was made package-private in modern avro
org.apache.avro.io.DecoderFactory only exists in avro 1.4+, but was not absolutely required under 1.4.
for broadest compatibility (and ability to reuse existing custom decoders for new InputStreams) use
com.linkedin.avroutil1.compatibility.AvroCompatibilityHelper.newBinaryDecoder()
</p>
]]>
</Details>
</BugPattern>

<!-- bug codes -->

<BugCode abbrev="BEI">BinaryEncoder instantiation</BugCode>
<BugCode abbrev="BDI">BinaryDecoder instantiation</BugCode>
</MessageCollection>
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
package com.linkedin.avroutil1.spotbugs;

import org.apache.avro.io.BinaryDecoder;
import org.apache.avro.io.BinaryEncoder;

public class BadClass {

public void instantiateBinaryEncoder() {
BinaryEncoder bobTheEncoder = new BinaryEncoder(null);
}

public void instantiateBinaryDecoder() {
BinaryDecoder bobTheDecoder = new BinaryDecoder(null);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package com.linkedin.avroutil1.spotbugs;

import edu.umd.cs.findbugs.BugCollection;
import edu.umd.cs.findbugs.test.SpotBugsRule;
import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcher;
import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder;
import org.hamcrest.MatcherAssert;
import org.junit.Rule;
import org.junit.Test;

import java.nio.file.Path;
import java.nio.file.Paths;

import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly;

public class BinaryDecoderInstantiationDetectorTest {
@Rule
public SpotBugsRule spotbugs = new SpotBugsRule();

@Test
public void testBadCase() {
Path path = Paths.get("build/classes/java/test", BadClass.class.getName().replace('.', '/') + ".class");
BugCollection bugCollection = spotbugs.performAnalysis(path);

BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder()
.bugType("BINARY_DECODER_INSTANTIATION")
.inMethod("instantiateBinaryDecoder").build();
MatcherAssert.assertThat(bugCollection, containsExactly(1, bugTypeMatcher));
}

@Test
public void testGoodCase() {
Path path = Paths.get("build/classes/java/test", GoodClass.class.getName().replace('.', '/') + ".class");
BugCollection bugCollection = spotbugs.performAnalysis(path);

BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder()
.bugType("BINARY_DECODER_INSTANTIATION")
.build();
MatcherAssert.assertThat(bugCollection, containsExactly(0, bugTypeMatcher));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@

import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly;

public class BinaryEncoderUsageDetectorTest {
public class BinaryEncoderInstantiationDetectorTest {
@Rule
public SpotBugsRule spotbugs = new SpotBugsRule();

Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,21 @@
package com.linkedin.avroutil1.spotbugs;

import com.linkedin.avroutil1.compatibility.AvroCompatibilityHelper;
import org.apache.avro.io.BinaryDecoder;
import org.apache.avro.io.BinaryEncoder;
import org.apache.avro.io.DecoderFactory;

import java.io.InputStream;
import java.io.OutputStream;

public class GoodClass {

public void instantiateBinaryEncoder() {
BinaryEncoder bobTheEncoder = AvroCompatibilityHelper.newBinaryEncoder( (OutputStream) null);
}

public void instantiateBinaryDecoder() {
BinaryDecoder bobTheDecoder = AvroCompatibilityHelper.newBinaryDecoder( (InputStream) null);
BinaryDecoder robertTheDecoder = DecoderFactory.defaultFactory().createBinaryDecoder(new byte[] {1, 2, 3}, null);
}
}

0 comments on commit a1fada4

Please sign in to comment.