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.

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

fix: Megolm sessions become invalid after restarting client #2013

Merged
merged 1 commit into from
Feb 4, 2025

Conversation

krille-chan
Copy link
Contributor

@krille-chan krille-chan commented Feb 3, 2025

This bug seems to be in the
sdk for 5 years already without
anyone noticing. The method
loadInboundGroupSession
seems to return the wrong
variable when loading the
session from the key. While
the outboundgroupsession
loading method relies on
an inbound group session, it
silently marks the outbound
group session as invalid on
every restart and creates a
new one. This means we never
reuse megolm sessions after
restarting the client.

Fixing this will probably reduce
the amount of megolm sessions
used in a conversation by a lot
which could improve the
performance and make the
key backup more reliable.

Closes https://github.com/famedly/product-management/issues/2840

It looks like the bug was introduced 3 years ago while doing the null safe migration. Before the method looked like this:

  /// Loads an inbound group session
  Future<SessionKey> loadInboundGroupSession(
      String roomId, String sessionId, String senderKey) async {
    if (roomId == null || sessionId == null || senderKey == null) {
      return null;
    }
    if (_inboundGroupSessions.containsKey(roomId) &&
        _inboundGroupSessions[roomId].containsKey(sessionId)) {
      final sess = _inboundGroupSessions[roomId][sessionId];
      if (sess.senderKey != senderKey && sess.senderKey.isNotEmpty) {
        return null; // sender keys do not match....better not do anything
      }
      return sess; // nothing to do
    }
    final session =
        await client.database?.getInboundGroupSession(roomId, sessionId);
    if (session == null) {
      return null;
    }
    final sess = SessionKey.fromDb(session, client.userID);
    if (!_inboundGroupSessions.containsKey(roomId)) {
      _inboundGroupSessions[roomId] = <String, SessionKey>{};
    }
    if (!sess.isValid ||
        (sess.senderKey.isNotEmpty && sess.senderKey != senderKey)) {
      return null;
    }
    _inboundGroupSessions[roomId][sessionId] = sess;
    return sess;
  }

So here when returning sess this variable actually hold the old inbound group session. After null safety migration it returned the wrong sess.

@krille-chan krille-chan requested a review from a team as a code owner February 3, 2025 08:12
@krille-chan krille-chan force-pushed the krille/fix-megolm-sessions-invalid-after-restart branch from 4c9b701 to 3cc4124 Compare February 3, 2025 08:14
Copy link
Contributor

@coder-with-a-bushido coder-with-a-bushido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :)

This bug seems to be in the
sdk for 5 years already without
anyone noticing. The method
loadInboundGroupSession
seems to return the wrong
variable when loading the
session from the key. While
the outboundgroupsession
loading method relies on
an inbound group session, it
silently marks the outbound
group session as invalid on
every restart and creates a
new one. This means we never
reuse megolm sessions after
restarting the client.

Fixing this will probably reduce
the amount of megolm sessions
used in a conversation by a lot
which could improve the
performance and make the
key backup more reliable.
@krille-chan krille-chan force-pushed the krille/fix-megolm-sessions-invalid-after-restart branch from 3cc4124 to 6cd40d3 Compare February 4, 2025 09:13
@krille-chan krille-chan enabled auto-merge February 4, 2025 09:13
@krille-chan krille-chan merged commit 51b1005 into main Feb 4, 2025
12 checks passed
@krille-chan krille-chan deleted the krille/fix-megolm-sessions-invalid-after-restart branch February 4, 2025 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants