This repository has been archived by the owner on Apr 15, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[v2.0] Introduce Topics #236
[v2.0] Introduce Topics #236
Changes from 23 commits
15eb1dc
8886b36
d79e56d
125f314
c6eda71
3fbf7ea
bd41e2b
8515d9d
6d904c5
c5c104c
cd30f54
27fe5ca
ea855fb
2b00e82
8aa1fa6
0e12d43
53cb461
05c7d84
d257605
3990b43
21afe44
79bd3ea
0e222b4
4c26b18
b068917
10f923f
f14f065
1194c8e
ef728fd
d301431
158b3f3
705daad
e514d50
5b837d5
91c0b37
a5f0ae1
d191de1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Topic does not need to be taken by value, and it is not
Copy
now so it should be taken by refThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we not do this for
Identifier
as well despite theCopy
? It's an extra allocation without it being necessary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as something is
Copy
, you can be certain it does not incur in a heap allocation (basically because that's a hard condition for types to be able to implementCopy
). Generally speaking,Copy
is a marker that says that "cloning" the type is essentially the same as moving the type (amemcpy
). For this reason,Copy
types can keep using the old variable, even after a move to another variable (aka the copy semantics). Types that don't implementCopy
have a more complex cloning (for example, because they handle resources, like the pointer to some heap allocated memory that tneeds to be freed when dropping the value), therefore move != clone anymore.For the types that don't implement
Copy
, it's important not to take ownership in function parameters if it's not needed, cause you are forcing a potentially costly clone unnecessarily. ForCopy
types, this clone is as cheap as a move, and the extra cost of the move vs reference, even if arguable, I'd say is under the ream of "profile before assuming anything" and definitely negligible in our case. So in this case ergonomics prevail.TL;DR: performance is not a factor in this decision. But still passing
Identifier
by reference would be valid if you prefer it (for coherence, or any other reason)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this limit coming from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When recovering a user we end up unmasking the stream of wrapped topics/cursors, but if you use the wrong password (as we do in the examples) it mixes up the unmasked
Size
value for number of topics, and theSize
within the topicByte
unwrap gets messed up as well. This results in an attempt to create aVec
with a size that exceeds its max allocation availability (https://doc.rust-lang.org/stable/std/vec/struct.Vec.html#method.reserve), resulting in a panic that we have no way to catch.One thing that threw me off was I tried to use
isize::MAX
and it still resulted in panics on occassion when the new size exceededu32::MAX
, so I made that the default max length. From aStreams
perspective, we have no instance whereBytes
should exceed 32Kb let aloneu32::MAX
so I thought that was an appropriate value to use.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You actually found an important attack vector, as the
Size
of ofBytes
is an user input after all. I thinku32::MAX
is still too high, but at least it creates an initial barrier, and u16::MAX would be too low (I wouldn't assume we'll always have the 32KB limit in the transport).There are actually 2 different cases: the attack vector case, where an actor tries to exploit the
Bytes
preallocation, and the incorrect password case that causes an unwrapping of an incorrect value of the Byte'sSize
. About the latter, It's a bad smell that something like this can happen at all, and does not speak very good about how the decoding is implemented. And the error that we are returning here has nothing to do with the true error. I suggest 2 improvements, besides thisu32::MAX
limit:ContentUnwrap<State>
, we should add a MAC right after absorbing the password to validate it and stop the unwrap if the password is incorrect.unwrap::Absorb<Bytes<T>>
,unwrap::Skip<Bytes<T>>
andunwrap::Mask<Bytes<T>>
we should check that theSize
is smaller than the remaining length of the context buffer. This would be a very effective way to protect about attempts to exploit theBytes
preallocation without having to set an static maximum that is too low.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
Mac
addition is simple enough, but the size check on stream will require a new method in theIStream
trait to check against the intended length. It'll just need to move theensure!
check inside thetry_advance()
function so that we can ensure the length doesn't exceed size without splitting the stream.