diff --git a/build.gradle.kts b/build.gradle.kts index 30606877d37..34964add571 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -15,7 +15,6 @@ plugins { alias(libs.plugins.shadow) apply false alias(libs.plugins.spotless) - id("com.github.spotbugs") version "6.5.11" id("io.github.gradle-nexus.publish-plugin") version "2.0.0" id("me.champeau.gradle.japicmp") version "0.4.3" id("me.champeau.jmh") version "0.7.3" apply false diff --git a/buildSrc/build.gradle.kts b/buildSrc/build.gradle.kts index a5444bfa1eb..8a4d0cf3ddc 100644 --- a/buildSrc/build.gradle.kts +++ b/buildSrc/build.gradle.kts @@ -1,3 +1,5 @@ +import org.gradle.api.attributes.java.TargetJvmVersion + plugins { `java-gradle-plugin` `kotlin-dsl` @@ -94,6 +96,12 @@ repositories { dependencies { implementation(gradleApi()) implementation(libs.forbiddenapis) + implementation(libs.spotbugs.gradle.plugin) { + // Select the plugin's Java 11 variant while buildSrc still has to targets Java 8 for muzzle workers. + attributes { + attribute(TargetJvmVersion.TARGET_JVM_VERSION_ATTRIBUTE, 11) + } + } implementation("net.bytebuddy", "byte-buddy-gradle-plugin", libs.versions.byte.buddy.get()) diff --git a/buildSrc/src/main/kotlin/dd-trace-java.conventions.forbidden-apis.gradle.kts b/buildSrc/src/main/kotlin/dd-trace-java.conventions.forbidden-apis.gradle.kts new file mode 100644 index 00000000000..f90d1cd2040 --- /dev/null +++ b/buildSrc/src/main/kotlin/dd-trace-java.conventions.forbidden-apis.gradle.kts @@ -0,0 +1,18 @@ +plugins { + java + id("de.thetaphi.forbiddenapis") +} + +forbiddenApis { + signaturesFiles = files(rootDir.resolve("gradle/forbiddenApiFilters/main.txt")) + ignoreSignaturesOfMissingClasses = true + ignoreFailures = false +} + +tasks.matching { it.name.startsWith("forbiddenApis") && it.name.endsWith("Test") }.configureEach { + enabled = false +} + +dependencies { + compileOnly(versionCatalogs.named("libs").findLibrary("forbiddenapis").get()) +} diff --git a/buildSrc/src/main/kotlin/dd-trace-java.conventions.spotbugs.gradle.kts b/buildSrc/src/main/kotlin/dd-trace-java.conventions.spotbugs.gradle.kts new file mode 100644 index 00000000000..bd3064f402c --- /dev/null +++ b/buildSrc/src/main/kotlin/dd-trace-java.conventions.spotbugs.gradle.kts @@ -0,0 +1,71 @@ +import com.github.spotbugs.snom.SpotBugsTask + +plugins { + java + id("com.github.spotbugs") +} + +spotbugs { + ignoreFailures = false + // Exclude known types and packages + excludeFilter = rootDir.resolve("gradle/spotbugFilters/exclude.xml") + showProgress = !providers.environmentVariable("CI").isPresent +} + +val productionTaskName = Regex("spotbugsMain(?:_java\\d+)?") + +tasks.withType().configureEach { + when { + project.path.startsWith(":dd-smoke-tests") -> { + enabled = false + } + productionTaskName.matches(name) -> { + omitVisitors = listOf( + "ConstructorThrow", + "DefaultEncodingDetector", + "DoInsideDoPrivileged", + "DontUseEnum", + "DroppedException", + "FindDeadLocalStores", + "FindHEmismatch", + "FindNullDeref", + "FindReturnRef", + "FindRunInvocations", + "FindUselessControlFlow", + "InitializationChain", + "LazyInit", + "LoadOfKnownNullValue", + "LostLoggerDueToWeakReference", + "MethodReturnCheck", + "MutableStaticFields", + "Naming", + "RuntimeExceptionCapture", + "SerializableIdiom", + "UnreadFields", + ) + } + else -> { + // static-analysis-rules skill: enable selected test tasks here with their own detector configuration. + enabled = false + } + } + + launcher = javaToolchains.launcherFor { + languageVersion = JavaLanguageVersion.current() + } + + val reportFile = layout.buildDirectory.file("reports/spotbugs/$name.html") + reports { + create("html") { + required = true + outputLocation = reportFile + setStylesheet("fancy-hist.xsl") + } + } +} + +val libs = versionCatalogs.named("libs") +dependencies { + compileOnly(libs.findLibrary("spotbugs-annotations").get()) + testImplementation(libs.findLibrary("jsr305").get()) +} diff --git a/buildSrc/src/main/kotlin/dd-trace-java.module.instrumentation.gradle.kts b/buildSrc/src/main/kotlin/dd-trace-java.module.instrumentation.gradle.kts index f5d797dfbe6..ad4f117ce52 100644 --- a/buildSrc/src/main/kotlin/dd-trace-java.module.instrumentation.gradle.kts +++ b/buildSrc/src/main/kotlin/dd-trace-java.module.instrumentation.gradle.kts @@ -2,7 +2,7 @@ import datadog.gradle.plugin.instrument.BuildTimeInstrumentationExtension plugins { id("dd-trace-java.conventions.java") - id("de.thetaphi.forbiddenapis") + id("dd-trace-java.conventions.forbidden-apis") } // These plugins are produced by buildSrc and are only available at runtime. @@ -26,10 +26,7 @@ configurations.named("muzzleBootstrap") { tasks.withType().configureEach { enabled = false } forbiddenApis { - signaturesFiles = files( - "$rootDir/gradle/forbiddenApiFilters/main.txt", - "$rootDir/gradle/forbiddenApiFilters/instrumentation.txt" - ) + signaturesFiles += files(rootDir.resolve("gradle/forbiddenApiFilters/instrumentation.txt")) } val libs = versionCatalogs.named("libs") diff --git a/buildSrc/src/test/kotlin/datadog/gradle/plugin/quality/ForbiddenApisConventionPluginTest.kt b/buildSrc/src/test/kotlin/datadog/gradle/plugin/quality/ForbiddenApisConventionPluginTest.kt new file mode 100644 index 00000000000..fef6bd17de4 --- /dev/null +++ b/buildSrc/src/test/kotlin/datadog/gradle/plugin/quality/ForbiddenApisConventionPluginTest.kt @@ -0,0 +1,155 @@ +package datadog.gradle.plugin.quality + +import datadog.gradle.plugin.GradleFixture +import org.assertj.core.api.Assertions.assertThat +import org.gradle.testkit.runner.TaskOutcome +import org.junit.jupiter.api.BeforeEach +import org.junit.jupiter.api.Test +import java.io.File + +class ForbiddenApisConventionPluginTest : GradleFixture() { + @BeforeEach + fun setup() { + writeSettings("""rootProject.name = "forbidden-apis-convention-test"""") + writeFile("gradle/libs.versions.toml", File("../gradle/libs.versions.toml").readText()) + writeFile( + "gradle/forbiddenApiFilters/main.txt", + """ + java.lang.System#exit(int) + missing.OptionalDependency + """ + ) + } + + @Test + fun `preserves defaults and name-based exclusions for late source sets and tasks`() { + addSubproject( + "library", + """ + import de.thetaphi.forbiddenapis.gradle.CheckForbiddenApis + + plugins { + id("dd-trace-java.conventions.forbidden-apis") + } + + sourceSets.create("latestDepTest") + sourceSets.create("main_java11") + sourceSets.create("testFixtures") + sourceSets.create("jmh") + tasks.register("forbiddenApisSyntheticTest") + tasks.register("unrelatedTest") + + tasks.register("verifyDefaults") { + doLast { + val mainFilter = rootProject.file("gradle/forbiddenApiFilters/main.txt") + tasks.withType().forEach { + check(it.signaturesFiles.files == setOf(mainFilter)) + check(it.ignoreSignaturesOfMissingClasses) + check(!it.ignoreFailures) + } + listOf("forbiddenApisTest", "forbiddenApisLatestDepTest", "forbiddenApisSyntheticTest").forEach { + check(!tasks.getByName(it).enabled) + } + listOf("forbiddenApisMain", "forbiddenApisMain_java11", "forbiddenApisTestFixtures", "forbiddenApisJmh", "unrelatedTest").forEach { + check(tasks.getByName(it).enabled) + } + val aggregate = tasks.getByName("forbiddenApis") + val checkTask = tasks.getByName("check") + check(aggregate in checkTask.taskDependencies.getDependencies(checkTask)) + check(tasks.getByName("forbiddenApisMain_java11") in aggregate.taskDependencies.getDependencies(aggregate)) + val dependency = configurations.getByName("compileOnly").dependencies.single { + it.group == "de.thetaphi" && it.name == "forbiddenapis" + } + check(dependency.version == libs.versions.forbiddenapis.get()) + check(configurations.getByName("runtimeClasspath").allDependencies.none { it.name == "forbiddenapis" }) + } + } + """ + ) + + val result = run(":library:verifyDefaults") + + assertThat(result.task(":library:verifyDefaults")?.outcome) + .describedAs(result.output).isEqualTo(TaskOutcome.SUCCESS) + } + + @Test + fun `preserves extension and typed task overrides`() { + writeFile("gradle/forbiddenApiFilters/instrumentation.txt", "java.lang.Runtime#exit(int)") + writeRootProject( + """ + import de.thetaphi.forbiddenapis.gradle.CheckForbiddenApis + + plugins { + id("dd-trace-java.conventions.forbidden-apis") + } + + forbiddenApis { + signaturesFiles += files("gradle/forbiddenApiFilters/instrumentation.txt") + ignoreFailures = true + } + tasks.named("forbiddenApisMain") { + failOnMissingClasses = false + } + tasks.named("forbiddenApisTest") { + enabled = true + } + + tasks.register("verifyOverrides") { + doLast { + val main = tasks.named("forbiddenApisMain").get() + check(main.signaturesFiles.files == setOf(file("gradle/forbiddenApiFilters/main.txt"), file("gradle/forbiddenApiFilters/instrumentation.txt"))) + check(main.ignoreFailures) + check(!main.failOnMissingClasses) + check(tasks.getByName("forbiddenApisTest").enabled) + } + } + """ + ) + + val result = run("verifyOverrides") + + assertThat(result.task(":verifyOverrides")?.outcome) + .describedAs(result.output).isEqualTo(TaskOutcome.SUCCESS) + } + + @Test + fun `fails check on a forbidden call and allows compile-only suppression`() { + writeRootProject( + """ + plugins { + id("dd-trace-java.conventions.forbidden-apis") + } + repositories { + mavenCentral() + } + """ + ) + writeJavaSource("Example", "public class Example { public void exit() { System.exit(0); } }") + + val failed = run("check", expectFailure = true) + + assertThat(failed.task(":forbiddenApisMain")?.outcome) + .describedAs(failed.output).isEqualTo(TaskOutcome.FAILED) + assertThat(failed.output).contains("java.lang.System#exit(int)") + + writeJavaSource( + "Example", + """ + import de.thetaphi.forbiddenapis.SuppressForbidden; + + public class Example { + @SuppressForbidden + public void exit() { System.exit(0); } + } + """ + ) + + val suppressed = run("check") + + assertThat(suppressed.task(":forbiddenApisMain")?.outcome) + .describedAs(suppressed.output).isEqualTo(TaskOutcome.SUCCESS) + assertThat(suppressed.task(":forbiddenApisTest")?.outcome).isEqualTo(TaskOutcome.SKIPPED) + assertThat(suppressed.task(":check")?.outcome).isEqualTo(TaskOutcome.SUCCESS) + } +} diff --git a/buildSrc/src/test/kotlin/datadog/gradle/plugin/spotbugs/SpotBugsConventionsPluginTest.kt b/buildSrc/src/test/kotlin/datadog/gradle/plugin/spotbugs/SpotBugsConventionsPluginTest.kt new file mode 100644 index 00000000000..559fd9a9465 --- /dev/null +++ b/buildSrc/src/test/kotlin/datadog/gradle/plugin/spotbugs/SpotBugsConventionsPluginTest.kt @@ -0,0 +1,169 @@ +package datadog.gradle.plugin.spotbugs + +import datadog.gradle.plugin.GradleFixture +import org.assertj.core.api.Assertions.assertThat +import org.gradle.testkit.runner.TaskOutcome.FAILED +import org.gradle.testkit.runner.TaskOutcome.SKIPPED +import org.gradle.testkit.runner.TaskOutcome.SUCCESS +import org.junit.jupiter.api.BeforeEach +import org.junit.jupiter.api.Test +import org.junit.jupiter.params.ParameterizedTest +import org.junit.jupiter.params.provider.ValueSource + +class SpotBugsConventionsPluginTest : GradleFixture() { + @BeforeEach + fun setup() { + writeSettings("""rootProject.name = "spotbugs-convention"""") + writeFile( + "gradle/libs.versions.toml", + """ + [libraries] + spotbugs-annotations = { module = "com.github.spotbugs:spotbugs-annotations", version = "4.10.4" } + jsr305 = { module = "com.google.code.findbugs:jsr305", version = "3.0.2" } + """ + ) + writeFile("gradle/spotbugFilters/exclude.xml", "") + writeRootProject( + """ + import com.github.spotbugs.snom.SpotBugsTask + + plugins { id("dd-trace-java.conventions.spotbugs") } + allprojects { repositories { mavenCentral() } } + tasks.withType().configureEach { options.release = 8 } + """ + ) + } + + @ParameterizedTest + @ValueSource(strings = ["main", "main_java6", "main_java11", "main_java17", "main_java21", "main_java25"]) + fun `production violations fail check and appear in the HTML report`(sourceSet: String) { + writeRootProject("""sourceSets.maybeCreate("$sourceSet")""", append = true) + writeViolation(sourceSet) + + val result = run("check", expectFailure = true) + val taskName = "spotbugs" + sourceSet.replaceFirstChar(Char::uppercaseChar) + + assertThat(result.task(":$taskName")?.outcome).describedAs(result.output).isEqualTo(FAILED) + assertThat(buildFile("reports/spotbugs/$taskName.html")) + .exists().content().contains("ES_COMPARING_PARAMETER_STRING_WITH_EQ") + } + + @Test + fun `test benchmark and smoke sources are not analysed`() { + val sourceSets = listOf("test", "integrationTest", "test_java17", "jmh", "otherMain") + sourceSets.forEach { + writeRootProject("""sourceSets.maybeCreate("$it")""", append = true) + writeViolation(it) + } + addSubproject( + "dd-smoke-tests:application", + """ + plugins { id("dd-trace-java.conventions.spotbugs") } + sourceSets.create("main_java17") + """ + ) + writeViolation("main", ":dd-smoke-tests:application") + writeViolation("main_java17", ":dd-smoke-tests:application") + val taskPaths = sourceSets.map { ":spotbugs" + it.replaceFirstChar(Char::uppercaseChar) } + + listOf( + ":dd-smoke-tests:application:spotbugsMain", + ":dd-smoke-tests:application:spotbugsMain_java17" + ) + + val result = run(*taskPaths.toTypedArray()) + + taskPaths.forEach { + assertThat(result.task(it)?.outcome).describedAs("$it\n${result.output}").isEqualTo(SKIPPED) + } + } + + @Test + fun `module analysis exclusions apply to existing and later production tasks`() { + writeRootProject( + """ + tasks.named("spotbugsMain").get() + spotbugs { onlyAnalyze = listOf("none") } + sourceSets.create("main_java11") + """, + append = true + ) + writeViolation("main") + writeViolation("main_java11") + + val result = run("spotbugsMain", "spotbugsMain_java11") + + listOf("spotbugsMain", "spotbugsMain_java11").forEach { + assertThat(result.task(":$it")?.outcome).describedAs(result.output).isEqualTo(SUCCESS) + assertThat(buildFile("reports/spotbugs/$it.html")) + .exists().content().doesNotContain("ES_COMPARING_PARAMETER_STRING_WITH_EQ") + } + } + + @Test + fun `test rules can change independently of production rules`() { + writeRootProject( + """ + tasks.named("spotbugsTest") { + enabled = true + visitors = listOf("FindNullDeref") + } + """, + append = true + ) + writeViolation("main") + writeJavaSource( + "example.NullDereference", + """ + package example; + public class NullDereference { + public int length() { + String value = null; + return value.length(); + } + } + """, + sourceSet = "test" + ) + + val nullCheck = run("spotbugsMain", "spotbugsTest", "--continue", expectFailure = true) + + assertThat(nullCheck.task(":spotbugsMain")?.outcome).describedAs(nullCheck.output).isEqualTo(FAILED) + assertThat(nullCheck.task(":spotbugsTest")?.outcome).describedAs(nullCheck.output).isEqualTo(FAILED) + assertThat(buildFile("reports/spotbugs/spotbugsTest.html")) + .exists().content().contains("NP_ALWAYS_NULL") + + writeRootProject( + """ + tasks.named("spotbugsTest") { + visitors = listOf("FindOpenStream") + } + """, + append = true + ) + + val resourceCheck = run("spotbugsMain", "spotbugsTest", "--continue", expectFailure = true) + + assertThat(resourceCheck.task(":spotbugsMain")?.outcome) + .describedAs(resourceCheck.output).isEqualTo(FAILED) + assertThat(resourceCheck.task(":spotbugsTest")?.outcome) + .describedAs(resourceCheck.output).isEqualTo(SUCCESS) + assertThat(buildFile("reports/spotbugs/spotbugsTest.html")) + .exists().content().doesNotContain("NP_ALWAYS_NULL") + } + + private fun writeViolation(sourceSet: String, projectPath: String? = null) { + writeJavaSource( + "example.StringComparison", + """ + package example; + public class StringComparison { + public boolean same(String left, String right) { + return left == right; + } + } + """, + sourceSet, + projectPath + ) + } +} diff --git a/dd-java-agent/instrumentation/karate/karate-2.0/src/main/java21/datadog/trace/instrumentation/karate2/TestEventsHandlerHolder.java b/dd-java-agent/instrumentation/karate/karate-2.0/src/main/java21/datadog/trace/instrumentation/karate2/TestEventsHandlerHolder.java index d2c4c495cab..c37323ec15d 100644 --- a/dd-java-agent/instrumentation/karate/karate-2.0/src/main/java21/datadog/trace/instrumentation/karate2/TestEventsHandlerHolder.java +++ b/dd-java-agent/instrumentation/karate/karate-2.0/src/main/java21/datadog/trace/instrumentation/karate2/TestEventsHandlerHolder.java @@ -8,7 +8,7 @@ public abstract class TestEventsHandlerHolder { - public static volatile TestEventsHandler TEST_EVENTS_HANDLER; + static volatile TestEventsHandler TEST_EVENTS_HANDLER; static { start(); diff --git a/gradle.lockfile b/gradle.lockfile deleted file mode 100644 index 62fccbf9c11..00000000000 --- a/gradle.lockfile +++ /dev/null @@ -1,29 +0,0 @@ -# This is a Gradle generated file for dependency locking. -# Manual edits can break the build and are not advised. -# This file is expected to be part of source control. -# To regenerate this file, run: ./gradlew :dependencies --write-locks -com.github.spotbugs:spotbugs-annotations:4.10.4=spotbugs -com.github.spotbugs:spotbugs:4.10.4=spotbugs -com.github.stephenc.jcip:jcip-annotations:1.0-1=spotbugs -com.google.code.findbugs:jsr305:3.0.2=spotbugs -com.google.code.gson:gson:2.14.0=spotbugs -com.google.errorprone:error_prone_annotations:2.48.0=spotbugs -commons-io:commons-io:2.21.0=spotbugs -jaxen:jaxen:2.0.6=spotbugs -net.sf.saxon:Saxon-HE:12.10=spotbugs -org.apache.bcel:bcel:6.12.0=spotbugs -org.apache.commons:commons-lang3:3.20.0=spotbugs -org.apache.commons:commons-text:1.15.0=spotbugs -org.apache.logging.log4j:log4j-api:2.26.1=spotbugs -org.apache.logging.log4j:log4j-core:2.26.1=spotbugs -org.dom4j:dom4j:2.2.0=spotbugs -org.ow2.asm:asm-analysis:9.10.1=spotbugs -org.ow2.asm:asm-commons:9.10.1=spotbugs -org.ow2.asm:asm-tree:9.10.1=spotbugs -org.ow2.asm:asm-util:9.10.1=spotbugs -org.ow2.asm:asm:9.10.1=spotbugs -org.slf4j:slf4j-api:2.0.17=spotbugsSlf4j -org.slf4j:slf4j-api:2.0.18=spotbugs -org.slf4j:slf4j-simple:2.0.17=spotbugsSlf4j -org.xmlresolver:xmlresolver:5.3.3=spotbugs -empty=spotbugsPlugins diff --git a/gradle/forbiddenapis.gradle b/gradle/forbiddenapis.gradle deleted file mode 100644 index 788d5b1b979..00000000000 --- a/gradle/forbiddenapis.gradle +++ /dev/null @@ -1,18 +0,0 @@ -apply plugin: "de.thetaphi.forbiddenapis" - -def mainFilterFile = files("$rootDir/gradle/forbiddenApiFilters/main.txt") - -forbiddenApis { - signaturesFiles = mainFilterFile - ignoreSignaturesOfMissingClasses = true - ignoreFailures = false -} - -// disable forbiddenApis for all test tasks -tasks.matching { it.name.startsWith('forbiddenApis') && it.name.endsWith('Test') }.configureEach { - it.enabled = false -} - -dependencies { - compileOnly libs.forbiddenapis -} diff --git a/gradle/java_no_deps.gradle b/gradle/java_no_deps.gradle index b723dd5e73c..f558fc04fb4 100644 --- a/gradle/java_no_deps.gradle +++ b/gradle/java_no_deps.gradle @@ -10,11 +10,11 @@ import java.nio.file.LinkOption apply plugin: 'dd-trace-java.configure-tests' apply plugin: 'java-library' apply plugin: 'dd-trace-java.test-jvm-constraints' +apply plugin: 'dd-trace-java.conventions.forbidden-apis' +apply plugin: 'dd-trace-java.conventions.spotbugs' apply from: "$rootDir/gradle/codenarc.gradle" -apply from: "$rootDir/gradle/forbiddenapis.gradle" apply from: "$rootDir/gradle/spotless.gradle" -apply from: "$rootDir/gradle/spotbugs.gradle" apply from: "$rootDir/gradle/repositories.gradle" apply from: "$rootDir/gradle/test-suites.gradle" diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 5ca41e88055..2905edf2053 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -7,6 +7,7 @@ forbiddenapis = "3.11" gradle-tooling-api = "8.14.5" jsr305 = "3.0.2" shadow = "9.4.2" +spotbugs-gradle-plugin = "6.5.11" spotbugs_annotations = "4.10.4" # Source code formatters @@ -94,6 +95,7 @@ forbiddenapis = { module = "de.thetaphi:forbiddenapis", version.ref = "forbidden gradle-tooling-api = { module = "org.gradle:gradle-tooling-api", version.ref = "gradle-tooling-api" } jsr305 = { module = "com.google.code.findbugs:jsr305", version.ref = "jsr305" } spotbugs-annotations = { module = "com.github.spotbugs:spotbugs-annotations", version.ref = "spotbugs_annotations" } +spotbugs-gradle-plugin = { module = "com.github.spotbugs.snom:spotbugs-gradle-plugin", version.ref = "spotbugs-gradle-plugin" } # DataDog libs and forks datadog-okhttp = { module = "com.datadoghq.okhttp3:okhttp", version.ref = "datadog-okhttp" } diff --git a/gradle/spotbugs.gradle b/gradle/spotbugs.gradle deleted file mode 100644 index a69304a7728..00000000000 --- a/gradle/spotbugs.gradle +++ /dev/null @@ -1,64 +0,0 @@ -apply plugin: 'com.github.spotbugs' - -spotbugs { - ignoreFailures = true - excludeFilter = file("$rootDir/gradle/spotbugFilters/exclude.xml") -} - -// Configure spotbugs for Main tasks and disable it for all other -// Can't use the class here due to being different class -tasks.matching { it.name.startsWith('spotbugs') }.configureEach { - // Needs Java 11+ to run spotbugs - it.launcher = javaToolchains.launcherFor { - it.languageVersion = JavaLanguageVersion.current() - } - - def name = it.name - if (project.path.startsWith(":dd-smoke-tests") - || !(name.endsWith("Main") - || name.endsWith("Main_java11")) - ) { - it.enabled = false - return - } - - it.showProgress = !providers.environmentVariable("CI").isPresent() - it.ignoreFailures = false - // detector documentation is in the following link: - // https://spotbugs-in-kengo-toda.readthedocs.io/en/lqc-list-detectors/detectors.html - it.omitVisitors = [ - 'ConstructorThrow', - 'DefaultEncodingDetector', - 'DoInsideDoPrivileged', - 'DontUseEnum', - 'DroppedException', - 'FindDeadLocalStores', - 'FindHEmismatch', - 'FindNullDeref', - 'FindReturnRef', - 'FindRunInvocations', - 'FindUselessControlFlow', - 'InitializationChain', - 'LazyInit', - 'LoadOfKnownNullValue', - 'LostLoggerDueToWeakReference', - 'MethodReturnCheck', - 'MutableStaticFields', - 'Naming', - 'RuntimeExceptionCapture', - 'SerializableIdiom', - 'UnreadFields', - ] - it.reports { - html { - required = true - destination(file("$buildDir/reports/spotbugs/${name}.html")) - stylesheet = 'fancy-hist.xsl' - } - } -} - -dependencies { - compileOnly(libs.spotbugs.annotations) - testImplementation(libs.jsr305) -} diff --git a/utils/socket-utils/src/main/java17/datadog/common/socket/TunnelingJdkSocket.java b/utils/socket-utils/src/main/java17/datadog/common/socket/TunnelingJdkSocket.java index a0487a4e944..355ffcebffb 100644 --- a/utils/socket-utils/src/main/java17/datadog/common/socket/TunnelingJdkSocket.java +++ b/utils/socket-utils/src/main/java17/datadog/common/socket/TunnelingJdkSocket.java @@ -3,12 +3,14 @@ import datadog.trace.api.internal.VisibleForTesting; import java.io.IOException; import java.io.InputStream; +import java.io.InterruptedIOException; import java.io.OutputStream; import java.net.InetAddress; import java.net.InetSocketAddress; import java.net.Socket; import java.net.SocketAddress; import java.net.SocketException; +import java.net.SocketTimeoutException; import java.net.StandardProtocolFamily; import java.net.StandardSocketOptions; import java.net.UnixDomainSocketAddress; @@ -21,6 +23,7 @@ import java.nio.file.Path; import java.util.Iterator; import java.util.Set; +import java.util.concurrent.TimeUnit; /** * Subtype UNIX socket for a higher-fidelity impersonation of TCP sockets. This is named "tunneling" @@ -43,8 +46,8 @@ final class TunnelingJdkSocket extends Socket { static final int DEFAULT_BUFFER_SIZE = 8192; // Indicate that the buffer size is not set by initializing to -1 - private int sendBufferSize = -1; - private int receiveBufferSize = -1; + private volatile int sendBufferSize = -1; + private volatile int receiveBufferSize = -1; TunnelingJdkSocket(final Path path) throws IOException, UnsupportedOperationException { this.unixSocketAddress = UnixDomainSocketAddress.of(path); @@ -236,46 +239,66 @@ public InputStream getInputStream() throws IOException { @Override public int read() throws IOException { byte[] nextByte = new byte[1]; - return (read(nextByte, 0, 1) == -1) ? -1 : (nextByte[0] & 0xFF); + return (read(nextByte, 0, 1) == 1) ? (nextByte[0] & 0xFF) : -1; } @Override public int read(byte[] b, int off, int len) throws IOException { + if (off < 0 || len < 0 || off > b.length - len) { + throw new IndexOutOfBoundsException(); + } + if (len == 0) { + return 0; + } if (isInputShutdown()) { return -1; } buffer.clear(); + buffer.limit(Math.min(buffer.capacity(), len)); + int readTimeout = timeout; + long start = System.nanoTime(); + long remaining = readTimeout; try { - int readyChannels = readSelector.select(timeout); - if (readyChannels == 0) { - if (isClosed() || !readSelector.isOpen()) { + while (true) { + int readyChannels = readSelector.select(remaining); + if (readyChannels == 0 && (isClosed() || !readSelector.isOpen())) { throw new SocketException("Socket is closed"); } - return 0; - } - - Set selectedKeys = readSelector.selectedKeys(); - // Multiple input streams share this selector, so serialize iteration and removal from - // its non-thread-safe selected-key set. - synchronized (selectedKeys) { - Iterator keyIterator = selectedKeys.iterator(); - while (keyIterator.hasNext()) { - SelectionKey key = keyIterator.next(); - keyIterator.remove(); - if (key.isReadable()) { - int r = unixSocketChannel.read(buffer); - if (r == -1) { - return -1; + if (readyChannels > 0) { + Set selectedKeys = readSelector.selectedKeys(); + // Multiple input streams share this selector, so serialize iteration and removal + // from its non-thread-safe selected-key set. + synchronized (selectedKeys) { + Iterator keyIterator = selectedKeys.iterator(); + while (keyIterator.hasNext()) { + SelectionKey key = keyIterator.next(); + keyIterator.remove(); + if (key.isReadable()) { + int r = unixSocketChannel.read(buffer); + if (r == -1) { + return -1; + } + if (r > 0) { + buffer.flip(); + buffer.get(b, off, r); + return r; + } + } } - buffer.flip(); - len = Math.min(r, len); - buffer.get(b, off, len); - return len; + } + } + if (Thread.currentThread().isInterrupted()) { + throw new InterruptedIOException("Read interrupted"); + } + // Selection can wake without data; retry within the original timeout. + if (readTimeout > 0) { + remaining = readTimeout - TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - start); + if (remaining <= 0) { + throw new SocketTimeoutException("Read timed out"); } } } - return 0; } catch (ClosedSelectorException | CancelledKeyException e) { SocketException socketException = new SocketException("Socket is closed"); socketException.initCause(e); diff --git a/utils/socket-utils/src/test/java/datadog/common/socket/TunnelingJdkSocketTest.java b/utils/socket-utils/src/test/java/datadog/common/socket/TunnelingJdkSocketTest.java index ef6741e35bb..ac0757b4e64 100644 --- a/utils/socket-utils/src/test/java/datadog/common/socket/TunnelingJdkSocketTest.java +++ b/utils/socket-utils/src/test/java/datadog/common/socket/TunnelingJdkSocketTest.java @@ -7,16 +7,18 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively; import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.junit.jupiter.api.Assertions.fail; import static org.junit.jupiter.api.condition.JRE.JAVA_16; import java.io.IOException; import java.io.InputStream; +import java.io.InterruptedIOException; import java.io.OutputStream; import java.net.InetSocketAddress; import java.net.SocketException; +import java.net.SocketTimeoutException; import java.net.StandardProtocolFamily; import java.net.UnixDomainSocketAddress; +import java.nio.ByteBuffer; import java.nio.channels.CancelledKeyException; import java.nio.channels.ClosedSelectorException; import java.nio.channels.SelectableChannel; @@ -168,39 +170,69 @@ public void testTimeout() throws Exception { clientSocket.setSoTimeout(testTimeout); assertEquals(testTimeout, clientSocket.getSoTimeout()); + clientSocket.selector.wakeup(); long startTime = System.currentTimeMillis(); - int readResult = inputStream.read(); + assertThrows(SocketTimeoutException.class, inputStream::read); long endTime = System.currentTimeMillis(); long readDuration = endTime - startTime; int timeVariance = 100; assertTrue(readDuration >= testTimeout && readDuration <= testTimeout + timeVariance); - assertEquals(0, readResult); int newTimeout = testTimeout / 2; clientSocket.setSoTimeout(newTimeout); assertEquals(newTimeout, clientSocket.getSoTimeout()); - assertTimeoutPreemptively(Duration.ofMillis(testTimeout), () -> inputStream.read()); + assertTimeoutPreemptively( + Duration.ofMillis(testTimeout), + () -> assertThrows(SocketTimeoutException.class, () -> inputStream.read(new byte[1]))); + + int invalidTimeout = -1; + assertThrows(IllegalArgumentException.class, () -> clientSocket.setSoTimeout(invalidTimeout)); // The socket should block indefinitely when timeout is set to 0, per // https://docs.oracle.com/en/java/javase/16/docs/api//java.base/java/net/Socket.html#setSoTimeout(int). int infiniteTimeout = 0; clientSocket.setSoTimeout(infiniteTimeout); assertEquals(infiniteTimeout, clientSocket.getSoTimeout()); + ExecutorService reader = Executors.newSingleThreadExecutor(); try { - assertTimeoutPreemptively(Duration.ofMillis(testTimeout), () -> inputStream.read()); - fail("Read should block indefinitely with infinite timeout"); - } catch (AssertionError e) { - // Expected + clientSocket.selector.wakeup(); + Future read = reader.submit(() -> inputStream.read()); + assertThrows(TimeoutException.class, () -> read.get(testTimeout, TimeUnit.MILLISECONDS)); + reader.shutdownNow(); + assertInstanceOf( + InterruptedIOException.class, + assertThrows(ExecutionException.class, () -> read.get(5, TimeUnit.SECONDS)).getCause()); + } finally { + reader.shutdownNow(); + clientSocket.close(); + assertTrue(reader.awaitTermination(5, TimeUnit.SECONDS)); } - int invalidTimeout = -1; - assertThrows(IllegalArgumentException.class, () -> clientSocket.setSoTimeout(invalidTimeout)); - - clientSocket.close(); assertThrows(SocketException.class, () -> clientSocket.setSoTimeout(testTimeout)); assertThrows(SocketException.class, clientSocket::getSoTimeout); } + @Test + public void testSingleByteReadsPreserveRemainingData() throws Exception { + try (TunnelingJdkSocket clientSocket = createClient()) { + server.serverThread.join(TimeUnit.SECONDS.toMillis(5)); + SocketChannel peer = server.acceptedChannel.get(); + assertNotNull(peer); + ByteBuffer bytes = ByteBuffer.wrap(new byte[] {0, (byte) 255}); + while (bytes.hasRemaining()) { + peer.write(bytes); + } + peer.shutdownOutput(); + + clientSocket.setSoTimeout(1000); + InputStream inputStream = clientSocket.getInputStream(); + assertEquals(0, inputStream.read(new byte[0])); + assertEquals(0, inputStream.read()); + assertEquals(255, inputStream.read()); + assertEquals(-1, inputStream.read()); + } + } + @Test public void testBufferSizes() throws Exception { TunnelingJdkSocket clientSocket = createClient();