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 handling of larger messages in ockam_node #2236

Closed
thomcc opened this issue Nov 19, 2021 · 2 comments · Fixed by #2609
Closed

Fix handling of larger messages in ockam_node #2236

thomcc opened this issue Nov 19, 2021 · 2 comments · Fixed by #2609

Comments

@thomcc
Copy link
Contributor

thomcc commented Nov 19, 2021

Currently, we don't handle messages above a certain size correctly in the rust https://github.com/ockam-network/ockam/blob/bbb2f85bbb561095497b701b50e8e005f301b518/implementations/rust/ockam/ockam_node/src/parser.rs#L17

This doesn't correctly implement the BARE variable length integer scheme described in https://www.ietf.org/archive/id/draft-devault-bare-01.txt:

Each octet of the encoded value has the most-significant bit set, except for the last octet. The remaining bits are the integer value in 7-bit groups, least-significant first.

(Note that this means the value we can't be higher than is 127, not 255 as mentioned in the linked source, because of how the encoding works — if it were 128 or larger, the top bit would be set, meaning we would only be supposed to use the bottom 7 from this octet, and check the next one for the rest.)

It's possible this is the cause of some of the errors that we're seeing in #1624 (it seems somewhat surprising to me that we wouldn't even have "in-order for the most part in practice", but I could be mistaken or even misunderstanding what's being described). Either way, we should fix it.

It's possible this can be done by just using serde_bare. Not sure. If not, the varint scheme isn't that complex.

@neil2468
Copy link
Contributor

Sounds interesting. Can I be assigned this?

@thomcc
Copy link
Contributor Author

thomcc commented Mar 30, 2022

All yours!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants