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 integer limit in PDU definitions #2486

Closed
wants to merge 1 commit into from
Closed

Fix integer limit in PDU definitions #2486

wants to merge 1 commit into from

Conversation

jplatte
Copy link
Contributor

@jplatte jplatte commented Apr 2, 2020

Resolves #2485.

@turt2live turt2live self-requested a review April 2, 2020 16:24
@turt2live
Copy link
Member

@jplatte can we get sign-off for this change please? Just need to leave a comment like the following:

Signed-off-by: Your Name <email@example.org>

@jplatte
Copy link
Contributor Author

jplatte commented Apr 2, 2020

Signed-off-by: Jonas Platte <jplatte+git@posteo.de>

@turt2live turt2live requested review from a team and removed request for turt2live April 2, 2020 20:10
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.

Having implied this is the right solution in #2485, I'm not sure it's as simple as all this. There are two problems:

  • There certainly exist rooms with depths greater than 2**53, and if we suddenly change the rules, they'll stop working (or at least they would, if we honoured the spec).
  • In general, having the spec differ from what synapse does in practice is unhelpful: it just means that other implementations either have to ignore the spec, or fail to interoperate with synapse.

The real solution here is to not have to rely on the validity of depth (see MSC1229); in the shorter term I'm not entirely sure. We could either leave it as-is for current room versions and change it for future room versions, or we could take the hit on breaking the rooms with depths greater than 2**53 (which will only be that way due to past malicious abuse) and change the implementation in synapse.

I think that changing the spec without making synapse match is the worst of all possible worlds.

@turt2live
Copy link
Member

We are fixing this in a different way, represented by #2540

@turt2live turt2live closed this May 20, 2020
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.

PDU definitions and appendix section about canonical JSON disagree on maximum integer value
3 participants