From 4ccb72c0f2119b6f2f30a764a097a8f5fd209f8f Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Date: Fri, 21 Jun 2024 13:55:43 +0100
Subject: [PATCH] Element-R: Fix resource leaks in verification logic (#4263)

* Move `RustVerificationRequest.onChange` out to a method

The only reason it was an inner function in the first place was to avoid
storing a reference in the class to `outgoingRequestProcessor`. That changed
with d1dec4cd0858.

* Fix reference cycles in rust verification code
---
 src/rust-crypto/verification.ts | 68 ++++++++++++++++++++-------------
 1 file changed, 42 insertions(+), 26 deletions(-)

diff --git a/src/rust-crypto/verification.ts b/src/rust-crypto/verification.ts
index d25e6b26d16..ca599f310cc 100644
--- a/src/rust-crypto/verification.ts
+++ b/src/rust-crypto/verification.ts
@@ -72,30 +72,40 @@ export class RustVerificationRequest
         private readonly supportedVerificationMethods: string[],
     ) {
         super();
-
         this.reEmitter = new TypedReEmitter(this);
 
-        const onChange = async (): Promise<void> => {
-            const verification: RustSdkCryptoJs.Qr | RustSdkCryptoJs.Sas | undefined = this.inner.getVerification();
-
-            // Set the _verifier object (wrapping the rust `Verification` as a js-sdk Verifier) if:
-            // - we now have a `Verification` where we lacked one before
-            // - we have transitioned from QR to SAS
-            // - we are verifying with SAS, but we need to replace our verifier with a new one because both parties
-            //   tried to start verification at the same time, and we lost the tie breaking
-            if (verification instanceof RustSdkCryptoJs.Sas) {
-                if (this._verifier === undefined || this._verifier instanceof RustQrCodeVerifier) {
-                    this.setVerifier(new RustSASVerifier(verification, this, outgoingRequestProcessor));
-                } else if (this._verifier instanceof RustSASVerifier) {
-                    this._verifier.replaceInner(verification);
-                }
-            } else if (verification instanceof RustSdkCryptoJs.Qr && this._verifier === undefined) {
-                this.setVerifier(new RustQrCodeVerifier(verification, outgoingRequestProcessor));
+        // Obviously, the Rust object maintains a reference to the callback function. If the callback function maintains
+        // a reference to the Rust object, then we have a reference cycle which means that `RustVerificationRequest`
+        // will never be garbage-collected, and hence the underlying rust object will never be freed.
+        //
+        // To avoid this reference cycle, use a weak reference in the callback function. If the `RustVerificationRequest`
+        // gets garbage-collected, then there is nothing to update!
+        const weakThis = new WeakRef(this);
+        inner.registerChangesCallback(async () => weakThis.deref()?.onChange());
+    }
+
+    /**
+     * Hook which is called when the underlying rust class notifies us that there has been a change.
+     */
+    private onChange(): void {
+        const verification: RustSdkCryptoJs.Qr | RustSdkCryptoJs.Sas | undefined = this.inner.getVerification();
+
+        // Set the _verifier object (wrapping the rust `Verification` as a js-sdk Verifier) if:
+        // - we now have a `Verification` where we lacked one before
+        // - we have transitioned from QR to SAS
+        // - we are verifying with SAS, but we need to replace our verifier with a new one because both parties
+        //   tried to start verification at the same time, and we lost the tie breaking
+        if (verification instanceof RustSdkCryptoJs.Sas) {
+            if (this._verifier === undefined || this._verifier instanceof RustQrCodeVerifier) {
+                this.setVerifier(new RustSASVerifier(verification, this, this.outgoingRequestProcessor));
+            } else if (this._verifier instanceof RustSASVerifier) {
+                this._verifier.replaceInner(verification);
             }
+        } else if (verification instanceof RustSdkCryptoJs.Qr && this._verifier === undefined) {
+            this.setVerifier(new RustQrCodeVerifier(verification, this.outgoingRequestProcessor));
+        }
 
-            this.emit(VerificationRequestEvent.Change);
-        };
-        inner.registerChangesCallback(onChange);
+        this.emit(VerificationRequestEvent.Change);
     }
 
     private setVerifier(verifier: RustSASVerifier | RustQrCodeVerifier): void {
@@ -473,9 +483,12 @@ abstract class BaseRustVerifer<InnerType extends RustSdkCryptoJs.Qr | RustSdkCry
         super();
 
         this.completionDeferred = defer();
-        inner.registerChangesCallback(async () => {
-            this.onChange();
-        });
+
+        // As with RustVerificationRequest, we need to avoid a reference cycle.
+        // See the comments in RustVerificationRequest.
+        const weakThis = new WeakRef(this);
+        inner.registerChangesCallback(async () => weakThis.deref()?.onChange());
+
         // stop the runtime complaining if nobody catches a failure
         this.completionDeferred.promise.catch(() => null);
     }
@@ -752,9 +765,12 @@ export class RustSASVerifier extends BaseRustVerifer<RustSdkCryptoJs.Sas> implem
     public replaceInner(inner: RustSdkCryptoJs.Sas): void {
         if (this.inner != inner) {
             this.inner = inner;
-            inner.registerChangesCallback(async () => {
-                this.onChange();
-            });
+
+            // As with RustVerificationRequest, we need to avoid a reference cycle.
+            // See the comments in RustVerificationRequest.
+            const weakThis = new WeakRef(this);
+            inner.registerChangesCallback(async () => weakThis.deref()?.onChange());
+
             // replaceInner will only get called if we started the verification at the same time as the other side, and we lost
             // the tie breaker.  So we need to re-accept their verification.
             this.sendAccept();