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 <adelel-beik@19LDN-MAC108.local>

Co-authored-by: Adel El-Beik <adelel-beik@19LDN-MAC108.local>

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'
     }

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<URL, Pair<URL, Attachment>>()
+
+    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
+    }
+}

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<StateAndRef<*>>()
+            val outputs = listOf(baseOutState, baseOutState.copy(notary = ALICE), baseOutState.copy(notary = BOB))
+            val commands = emptyList<CommandWithParties<CommandData>>()
+
+            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<Attachment> {
+
+        val attachment = object : AbstractAttachment({contractJarPath.inputStream().readBytes()}, uploader = "app") {
+            @Suppress("OverridingDeprecatedMember")
+            override val signers: List<Party> = emptyList()
+            override val signerKeys: List<PublicKey> = 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<Party> = emptyList()
+                    override val signerKeys: List<PublicKey> = 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<Party> = emptyList()
+                    override val signerKeys: List<PublicKey> = emptyList()
+                    override val size: Int = 1234
+                    override val id: SecureHash = SecureHash.sha256(attachmentData)
+        },
+                contractAttachment)
+    }
 }

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<Attachment>,
         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: AttachmentsHolder = AttachmentsHolderImpl()
-    private val loadedAttachments = mutableMapOf<String, Attachment>().toSynchronised()
 
     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()
+        val proposedURL = URL(attachmentScheme, "", -1, attachment.id.toString(), AttachmentURLStreamHandler)
-        loadedAttachments[id] = attachment
+        val existingURL = loadedAttachments.getKey(proposedURL)
-        return URL(attachmentScheme, "", -1, id, AttachmentURLStreamHandler)
+        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 equals(attachmentUrl: URL, otherURL: URL?): Boolean {
-        override fun getContentLengthLong(): Long = attachment.size.toLong()
+            if (attachmentUrl.protocol != otherURL?.protocol) return false
-        override fun getInputStream(): InputStream = attachment.open()
+            if (attachmentUrl.protocol != attachmentScheme) throw IllegalArgumentException("Cannot handle protocol: ${attachmentUrl.protocol}")
-        override fun connect() {
+            return attachmentUrl.file == otherURL?.file
-            connected = true
+        }
+
+        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
+            }
         }
     }
 }
+
+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<URL, Pair<WeakReference<URL>, 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
+    }
+}

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<String>, versionSeed: Int = 0): Path {
+    private fun createTestClass(workingDir: Path, className: String, packages: List<String>, versionSeed: Int = 0, content: String? = null): Path {
-        val newClass = """package ${packages.joinToString(".")};
+        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)
     }

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<Attributes.Name, String>) {
+        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)
     }
 }