Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix recursion error when fetching auth chain over federation #7817

Merged
merged 9 commits into from
Jul 10, 2020

Conversation

erikjohnston
Copy link
Member

When fetching the state of a room over federation we receive the event
IDs of the state and auth chain. We then fetch those events that we
don't already have.

However, we used a function that recursively fetched any missing auth
events for the fetched events, which can lead to a lot of recursion if
the server is missing most of the auth chain. This work is entirely
pointless because would have queued up the missing events in the auth
chain to be fetched already.

Let's just diable the recursion, since it only gets called from one
place anyway.

When fetching the state of a room over federation we receive the event
IDs of the state and auth chain. We then fetch those events that we
don't already have.

However, we used a function that recursively fetched any missing auth
events for the fetched events, which can lead to a lot of recursion if
the server is missing most of the auth chain. This work is entirely
pointless because would have queued up the missing events in the auth
chain to be fetched already.

Let's just diable the recursion, since it only gets called from one
place anyway.
@babolivier babolivier self-requested a review July 10, 2020 10:06
Copy link
Contributor

@babolivier babolivier left a comment

Choose a reason for hiding this comment

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

lgtm otherwise, though I'd like @richvdh seal of approval before merging as he usually has more context than me on this area.

synapse/handlers/federation.py Outdated Show resolved Hide resolved
synapse/handlers/federation.py Outdated Show resolved Hide resolved
@erikjohnston
Copy link
Member Author

Tests are fixed by matrix-org/sytest#911

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm otherwise

synapse/handlers/federation.py Outdated Show resolved Hide resolved
synapse/handlers/federation.py Outdated Show resolved Hide resolved
erikjohnston and others added 3 commits July 10, 2020 16:26
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
@@ -69,10 +69,11 @@ def check(
# sanity-check it
for auth_event in auth_events.values():
if auth_event.room_id != room_id:
raise Exception(
raise AuthError(
500,
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be a 403?

Copy link
Member

Choose a reason for hiding this comment

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

oh oof there is AuthError(500) above. whatever.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, yes, probably actually. I was copying it from a previous line, but I don't think that makes sense

Copy link
Member

Choose a reason for hiding this comment

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

but also: if we actually hit this code and it matters, the comment needs changing.

Copy link
Member

Choose a reason for hiding this comment

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

/me awards past me a prize for including sanity checks for security-critical things

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, which comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

Found it

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm

@erikjohnston erikjohnston merged commit e29c443 into develop Jul 10, 2020
@erikjohnston erikjohnston deleted the erikj/fix_recursion_error branch July 10, 2020 17:15
babolivier pushed a commit that referenced this pull request Sep 1, 2021
* commit 'e29c44340':
  Fix recursion error when fetching auth chain over federation (#7817)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants