From a33309a31bd2fc93a936eb060335fda66839dabf Mon Sep 17 00:00:00 2001 From: Adel El-Beik <48713346+adelel1@users.noreply.github.com> Date: Wed, 27 May 2020 11:35:15 +0100 Subject: [PATCH] CORDA-3755: Backport AttachmentURLStreamHandlerFactory memory leak (#6274) * CORDA-3755: Switched attachments map to a WeakHashMap (#6214) * CORDA-3772: Now specify source and target of 8 when compiling contract classes. * CORDA-3651: addManifest now uses separate files for reading and writing. (#6026) * CORDA-3651: addManifest now uses separate files for reading and writing. * CORDA-3651: The jar scanning loader now closes itsself. Co-authored-by: Adel El-Beik Co-authored-by: Adel El-Beik --- core-deterministic/build.gradle | 1 + .../internal/AttachmentsHolderImpl.kt | 23 ++++ .../AttachmentsClassLoaderTests.kt | 126 +++++++++++++++++- .../internal/AttachmentsClassLoader.kt | 79 ++++++++--- .../core/internal/ContractJarTestUtils.kt | 12 +- .../core/internal/JarSignatureTestUtils.kt | 5 +- 6 files changed, 218 insertions(+), 28 deletions(-) create mode 100644 core-deterministic/src/main/kotlin/net/corda/core/serialization/internal/AttachmentsHolderImpl.kt diff --git a/core-deterministic/build.gradle b/core-deterministic/build.gradle index 4b84f32d7e..5d7d1cd971 100644 --- a/core-deterministic/build.gradle +++ b/core-deterministic/build.gradle @@ -59,6 +59,7 @@ task patchCore(type: Zip, dependsOn: coreJarTask) { from(zipTree(originalJar)) { exclude 'net/corda/core/internal/*ToggleField*.class' exclude 'net/corda/core/serialization/*SerializationFactory*.class' + exclude 'net/corda/core/serialization/internal/AttachmentsHolderImpl.class' exclude 'net/corda/core/serialization/internal/CheckpointSerializationFactory*.class' exclude 'net/corda/core/internal/rules/*.class' } diff --git a/core-deterministic/src/main/kotlin/net/corda/core/serialization/internal/AttachmentsHolderImpl.kt b/core-deterministic/src/main/kotlin/net/corda/core/serialization/internal/AttachmentsHolderImpl.kt new file mode 100644 index 0000000000..a2f7b8ab30 --- /dev/null +++ b/core-deterministic/src/main/kotlin/net/corda/core/serialization/internal/AttachmentsHolderImpl.kt @@ -0,0 +1,23 @@ +package net.corda.core.serialization.internal + +import net.corda.core.contracts.Attachment +import java.net.URL + +@Suppress("unused") +private class AttachmentsHolderImpl : AttachmentsHolder { + private val attachments = LinkedHashMap>() + + override val size: Int get() = attachments.size + + override fun getKey(key: URL): URL? { + return attachments[key]?.first + } + + override fun get(key: URL): Attachment? { + return attachments[key]?.second + } + + override fun set(key: URL, value: Attachment) { + attachments[key] = key to value + } +} diff --git a/core-tests/src/test/kotlin/net/corda/coretests/transactions/AttachmentsClassLoaderTests.kt b/core-tests/src/test/kotlin/net/corda/coretests/transactions/AttachmentsClassLoaderTests.kt index 9007a7e28d..3bcc21dd9a 100644 --- a/core-tests/src/test/kotlin/net/corda/coretests/transactions/AttachmentsClassLoaderTests.kt +++ b/core-tests/src/test/kotlin/net/corda/coretests/transactions/AttachmentsClassLoaderTests.kt @@ -1,19 +1,37 @@ package net.corda.coretests.transactions +import net.corda.core.contracts.AlwaysAcceptAttachmentConstraint import net.corda.core.contracts.Attachment +import net.corda.core.contracts.CommandData +import net.corda.core.contracts.CommandWithParties import net.corda.core.contracts.Contract +import net.corda.core.contracts.ContractAttachment +import net.corda.core.contracts.PrivacySalt +import net.corda.core.contracts.StateAndRef +import net.corda.core.contracts.TimeWindow +import net.corda.core.contracts.TransactionState import net.corda.core.contracts.TransactionVerificationException import net.corda.core.crypto.Crypto import net.corda.core.crypto.SecureHash +import net.corda.core.identity.Party +import net.corda.core.internal.AbstractAttachment import net.corda.core.internal.AttachmentTrustCalculator +import net.corda.core.internal.createLedgerTransaction import net.corda.core.internal.declaredField import net.corda.core.internal.hash import net.corda.core.internal.inputStream import net.corda.core.node.NetworkParameters import net.corda.core.node.services.AttachmentId import net.corda.core.serialization.internal.AttachmentsClassLoader -import net.corda.node.services.attachments.NodeAttachmentTrustCalculator import net.corda.testing.common.internal.testNetworkParameters +import net.corda.node.services.attachments.NodeAttachmentTrustCalculator +import net.corda.testing.contracts.DummyContract +import net.corda.testing.core.ALICE_NAME +import net.corda.testing.core.BOB_NAME +import net.corda.testing.core.DUMMY_NOTARY_NAME +import net.corda.testing.core.SerializationEnvironmentRule +import net.corda.testing.core.TestIdentity +import net.corda.testing.core.internal.ContractJarTestUtils import net.corda.testing.core.internal.ContractJarTestUtils.signContractJar import net.corda.testing.internal.TestingNamedCacheFactory import net.corda.testing.internal.fakeAttachment @@ -24,10 +42,14 @@ import org.apache.commons.io.IOUtils import org.junit.Assert.assertArrayEquals import org.junit.Assert.assertEquals import org.junit.Before +import org.junit.Rule import org.junit.Test +import org.junit.rules.TemporaryFolder import java.io.ByteArrayOutputStream import java.io.InputStream import java.net.URL +import java.nio.file.Path +import java.security.PublicKey import kotlin.test.assertFailsWith class AttachmentsClassLoaderTests { @@ -43,8 +65,21 @@ class AttachmentsClassLoaderTests { it.toByteArray() } } + val ALICE = TestIdentity(ALICE_NAME, 70).party + val BOB = TestIdentity(BOB_NAME, 80).party + val dummyNotary = TestIdentity(DUMMY_NOTARY_NAME, 20) + val DUMMY_NOTARY get() = dummyNotary.party + val PROGRAM_ID: String = "net.corda.testing.contracts.MyDummyContract" } + @Rule + @JvmField + val tempFolder = TemporaryFolder() + + @Rule + @JvmField + val testSerialization = SerializationEnvironmentRule(true) + private lateinit var storage: MockAttachmentStorage private lateinit var internalStorage: InternalMockAttachmentStorage private lateinit var attachmentTrustCalculator: AttachmentTrustCalculator @@ -448,4 +483,93 @@ class AttachmentsClassLoaderTests { createClassloader(trustedAttachment) } + + @Test(timeout=300_000) + fun `attachment still available in verify after forced gc in verify`() { + tempFolder.root.toPath().let { path -> + val baseOutState = TransactionState(DummyContract.SingleOwnerState(0, ALICE), PROGRAM_ID, DUMMY_NOTARY, constraint = AlwaysAcceptAttachmentConstraint) + val inputs = emptyList>() + val outputs = listOf(baseOutState, baseOutState.copy(notary = ALICE), baseOutState.copy(notary = BOB)) + val commands = emptyList>() + + val content = createContractString(PROGRAM_ID) + val contractJarPath = ContractJarTestUtils.makeTestContractJar(path, PROGRAM_ID, content = content) + + val attachments = createAttachments(contractJarPath) + + val id = SecureHash.randomSHA256() + val timeWindow: TimeWindow? = null + val privacySalt = PrivacySalt() + val transaction = createLedgerTransaction( + inputs, + outputs, + commands, + attachments, + id, + null, + timeWindow, + privacySalt, + testNetworkParameters(), + emptyList(), + isAttachmentTrusted = { true } + ) + transaction.verify() + } + } + + private fun createContractString(contractName: String, versionSeed: Int = 0): String { + val pkgs = contractName.split(".") + val className = pkgs.last() + val packages = pkgs.subList(0, pkgs.size - 1) + + val output = """package ${packages.joinToString(".")}; + import net.corda.core.contracts.*; + import net.corda.core.transactions.*; + import java.net.URL; + import java.io.InputStream; + + public class $className implements Contract { + private int seed = $versionSeed; + @Override + public void verify(LedgerTransaction tx) throws IllegalArgumentException { + System.gc(); + InputStream str = this.getClass().getClassLoader().getResourceAsStream("importantDoc.pdf"); + if (str == null) throw new IllegalStateException("Could not find importantDoc.pdf"); + } + } + """.trimIndent() + + System.out.println(output) + return output + } + + private fun createAttachments(contractJarPath: Path) : List { + + val attachment = object : AbstractAttachment({contractJarPath.inputStream().readBytes()}, uploader = "app") { + @Suppress("OverridingDeprecatedMember") + override val signers: List = emptyList() + override val signerKeys: List = emptyList() + override val size: Int = 1234 + override val id: SecureHash = SecureHash.sha256(attachmentData) + } + val contractAttachment = ContractAttachment(attachment, PROGRAM_ID) + + return listOf( + object : AbstractAttachment({ISOLATED_CONTRACTS_JAR_PATH.openStream().readBytes()}, uploader = "app") { + @Suppress("OverridingDeprecatedMember") + override val signers: List = emptyList() + override val signerKeys: List = emptyList() + override val size: Int = 1234 + override val id: SecureHash = SecureHash.sha256(attachmentData) + }, + object : AbstractAttachment({fakeAttachment("importantDoc.pdf", "I am a pdf!").inputStream().readBytes() + }, uploader = "app") { + @Suppress("OverridingDeprecatedMember") + override val signers: List = emptyList() + override val signerKeys: List = emptyList() + override val size: Int = 1234 + override val id: SecureHash = SecureHash.sha256(attachmentData) + }, + contractAttachment) + } } diff --git a/core/src/main/kotlin/net/corda/core/serialization/internal/AttachmentsClassLoader.kt b/core/src/main/kotlin/net/corda/core/serialization/internal/AttachmentsClassLoader.kt index e85e5f838f..a0f6f6c141 100644 --- a/core/src/main/kotlin/net/corda/core/serialization/internal/AttachmentsClassLoader.kt +++ b/core/src/main/kotlin/net/corda/core/serialization/internal/AttachmentsClassLoader.kt @@ -17,6 +17,7 @@ import net.corda.core.utilities.debug import java.io.ByteArrayOutputStream import java.io.IOException import java.io.InputStream +import java.lang.ref.WeakReference import java.net.* import java.util.* import java.util.jar.JarInputStream @@ -53,14 +54,6 @@ class AttachmentsClassLoader(attachments: List, private val ignoreDirectories = listOf("org/jolokia/", "org/json/simple/") private val ignorePackages = ignoreDirectories.map { it.replace("/", ".") } - @VisibleForTesting - private fun readAttachment(attachment: Attachment, filepath: String): ByteArray { - ByteArrayOutputStream().use { - attachment.extractFile(filepath, it) - return it.toByteArray() - } - } - /** * Apply our custom factory either directly, if `URL.setURLStreamHandlerFactory` has not been called yet, * or use a decorator and reflection to bypass the single-call-per-JVM restriction otherwise. @@ -354,8 +347,7 @@ object AttachmentsClassLoaderBuilder { object AttachmentURLStreamHandlerFactory : URLStreamHandlerFactory { private const val attachmentScheme = "attachment" - // TODO - what happens if this grows too large? - private val loadedAttachments = mutableMapOf().toSynchronised() + private val loadedAttachments: AttachmentsHolder = AttachmentsHolderImpl() override fun createURLStreamHandler(protocol: String): URLStreamHandler? { return if (attachmentScheme == protocol) { @@ -363,25 +355,70 @@ object AttachmentURLStreamHandlerFactory : URLStreamHandlerFactory { } else null } + @Synchronized fun toUrl(attachment: Attachment): URL { - val id = attachment.id.toString() - loadedAttachments[id] = attachment - return URL(attachmentScheme, "", -1, id, AttachmentURLStreamHandler) + val proposedURL = URL(attachmentScheme, "", -1, attachment.id.toString(), AttachmentURLStreamHandler) + val existingURL = loadedAttachments.getKey(proposedURL) + return if (existingURL == null) { + loadedAttachments[proposedURL] = attachment + proposedURL + } else { + existingURL + } } + @VisibleForTesting + fun loadedAttachmentsSize(): Int = loadedAttachments.size + private object AttachmentURLStreamHandler : URLStreamHandler() { override fun openConnection(url: URL): URLConnection { if (url.protocol != attachmentScheme) throw IOException("Cannot handle protocol: ${url.protocol}") - val attachment = loadedAttachments[url.path] ?: throw IOException("Could not load url: $url .") + val attachment = loadedAttachments[url] ?: throw IOException("Could not load url: $url .") return AttachmentURLConnection(url, attachment) } - } - private class AttachmentURLConnection(url: URL, private val attachment: Attachment) : URLConnection(url) { - override fun getContentLengthLong(): Long = attachment.size.toLong() - override fun getInputStream(): InputStream = attachment.open() - override fun connect() { - connected = true + override fun equals(attachmentUrl: URL, otherURL: URL?): Boolean { + if (attachmentUrl.protocol != otherURL?.protocol) return false + if (attachmentUrl.protocol != attachmentScheme) throw IllegalArgumentException("Cannot handle protocol: ${attachmentUrl.protocol}") + return attachmentUrl.file == otherURL?.file + } + + override fun hashCode(url: URL): Int { + if (url.protocol != attachmentScheme) throw IllegalArgumentException("Cannot handle protocol: ${url.protocol}") + return url.file.hashCode() + } + + private class AttachmentURLConnection(url: URL, private val attachment: Attachment) : URLConnection(url) { + override fun getContentLengthLong(): Long = attachment.size.toLong() + override fun getInputStream(): InputStream = attachment.open() + override fun connect() { + connected = true + } } } -} \ No newline at end of file +} + +interface AttachmentsHolder { + val size: Int + fun getKey(key: URL): URL? + operator fun get(key: URL): Attachment? + operator fun set(key: URL, value: Attachment) +} + +private class AttachmentsHolderImpl : AttachmentsHolder { + private val attachments = WeakHashMap, Attachment>>().toSynchronised() + + override val size: Int get() = attachments.size + + override fun getKey(key: URL): URL? { + return attachments[key]?.first?.get() + } + + override fun get(key: URL): Attachment? { + return attachments[key]?.second + } + + override fun set(key: URL, value: Attachment) { + attachments[key] = WeakReference(key) to value + } +} diff --git a/testing/test-utils/src/main/kotlin/net/corda/testing/core/internal/ContractJarTestUtils.kt b/testing/test-utils/src/main/kotlin/net/corda/testing/core/internal/ContractJarTestUtils.kt index cefc596c3a..3ba57fa20a 100644 --- a/testing/test-utils/src/main/kotlin/net/corda/testing/core/internal/ContractJarTestUtils.kt +++ b/testing/test-utils/src/main/kotlin/net/corda/testing/core/internal/ContractJarTestUtils.kt @@ -59,12 +59,14 @@ object ContractJarTestUtils { return workingDir.resolve(jarName) to signer } + @Suppress("LongParameterList") @JvmOverloads - fun makeTestContractJar(workingDir: Path, contractName: String, signed: Boolean = false, version: Int = 1, versionSeed: Int = 0): Path { + fun makeTestContractJar(workingDir: Path, contractName: String, signed: Boolean = false, version: Int = 1, versionSeed: Int = 0, + content: String? = null): Path { val packages = contractName.split(".") val jarName = "attachment-${packages.last()}-$version-$versionSeed-${(if (signed) "signed" else "")}.jar" val className = packages.last() - createTestClass(workingDir, className, packages.subList(0, packages.size - 1), versionSeed) + createTestClass(workingDir, className, packages.subList(0, packages.size - 1), versionSeed, content) workingDir.createJar(jarName, "${contractName.replace(".", "/")}.class") workingDir.addManifest(jarName, Pair(Attributes.Name(CORDAPP_CONTRACT_VERSION), version.toString())) return workingDir.resolve(jarName) @@ -87,8 +89,8 @@ object ContractJarTestUtils { return workingDir.resolve(jarName) } - private fun createTestClass(workingDir: Path, className: String, packages: List, versionSeed: Int = 0): Path { - val newClass = """package ${packages.joinToString(".")}; + private fun createTestClass(workingDir: Path, className: String, packages: List, versionSeed: Int = 0, content: String? = null): Path { + val newClass = content ?: """package ${packages.joinToString(".")}; import net.corda.core.contracts.*; import net.corda.core.transactions.*; @@ -108,7 +110,7 @@ object ContractJarTestUtils { val fileManager = compiler.getStandardFileManager(null, null, null) fileManager.setLocation(StandardLocation.CLASS_OUTPUT, listOf(workingDir.toFile())) - compiler.getTask(System.out.writer(), fileManager, null, null, null, listOf(source)).call() + compiler.getTask(System.out.writer(), fileManager, null, listOf("-source", "8", "-target", "8"), null, listOf(source)).call() val outFile = fileManager.getFileForInput(StandardLocation.CLASS_OUTPUT, packages.joinToString("."), "$className.class") return Paths.get(outFile.name) } diff --git a/testing/test-utils/src/main/kotlin/net/corda/testing/core/internal/JarSignatureTestUtils.kt b/testing/test-utils/src/main/kotlin/net/corda/testing/core/internal/JarSignatureTestUtils.kt index 00a827c1e1..4c21340f84 100644 --- a/testing/test-utils/src/main/kotlin/net/corda/testing/core/internal/JarSignatureTestUtils.kt +++ b/testing/test-utils/src/main/kotlin/net/corda/testing/core/internal/JarSignatureTestUtils.kt @@ -12,6 +12,7 @@ import java.nio.file.Files import java.nio.file.NoSuchFileException import java.nio.file.Path import java.nio.file.Paths +import java.nio.file.StandardCopyOption.REPLACE_EXISTING import java.security.PublicKey import java.util.jar.Attributes import java.util.jar.JarInputStream @@ -88,12 +89,13 @@ object JarSignatureTestUtils { JarInputStream(FileInputStream((this / fileName).toFile())).use(JarSignatureCollector::collectSigners) fun Path.addManifest(fileName: String, vararg entries: Pair) { + val outputFile = this / (fileName + "Output") JarInputStream(FileInputStream((this / fileName).toFile())).use { input -> val manifest = input.manifest ?: Manifest() entries.forEach { (attributeName, value) -> manifest.mainAttributes[attributeName] = value } - val output = JarOutputStream(FileOutputStream((this / fileName).toFile()), manifest) + val output = JarOutputStream(FileOutputStream(outputFile.toFile()), manifest) var entry = input.nextEntry val buffer = ByteArray(1 shl 14) while (true) { @@ -108,5 +110,6 @@ object JarSignatureTestUtils { } output.close() } + Files.copy(outputFile, this / fileName, REPLACE_EXISTING) } }