Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Element-R: Fix resource leaks in verification logic #4263

Merged
merged 3 commits into from
Jun 21, 2024
Merged

Element-R: Fix resource leaks in verification logic #4263

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5109cf1
Move `RustVerificationRequest.onChange` out to a method
richvdh Jun 21, 2024
dc2bed9
Fix reference cycles in rust verification code
richvdh Jun 21, 2024
13e8ccc
Merge branch 'develop' into rav/element-r/fix_verification_leak
richvdh Jun 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 42 additions & 26 deletions src/rust-crypto/verification.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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();
Expand Down
Loading