Skip to content

Fix RingBuffer::extend_from_within_unchecked unsoundness#76

Merged
KillingSpark merged 3 commits intoKillingSpark:masterfrom
paolobarbolini:fix-unsoundness
Nov 28, 2024
Merged

Fix RingBuffer::extend_from_within_unchecked unsoundness#76
KillingSpark merged 3 commits intoKillingSpark:masterfrom
paolobarbolini:fix-unsoundness

Conversation

@paolobarbolini
Copy link
Contributor

@paolobarbolini paolobarbolini commented Nov 27, 2024

This fixes the unsoundness reported in #75 by correcting the src length math. Also adds safety and general comments, making it much easier to understand the logic.

The first commit fixes the vulnerability while the other commits cleanup the implementation and add two unrelated debug assertions.

Fixes #75

Copy link

@StarStarJ StarStarJ left a comment

Choose a reason for hiding this comment

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

Works for my workflow with this patch.

I don't know the code of this crate, so I cannot really help with a proper review.
But thanks for taking care :D

@KillingSpark
Copy link
Owner

KillingSpark commented Nov 28, 2024

Thanks so much for fixing this!

The text comments do add a lot of clarification. I find the diagrams a bit hard to read especially when they get to four lines of ^ markers.

Maybe something like this would be better?

|___HXXXSSSXXXXXXXTDD__|
H: Head position (first readable byte)
T: Tail position (first writeable byte)
X: Uninvolved bytes in the readable section
S: Source bytes, to be copied at the end of the buffer
D: Destination bytes, going to be written to by copying the source bytes
_: Uninvolved bytes in the writable section

|D__HXXXSSSSSSSXXXTDDDD|

|XXSSSSSXXXXXTDDDD__HXX|

|SSSXXTDDDD__HXXXXXXXSS|

@paolobarbolini
Copy link
Contributor Author

What if we had something like this? To me it looks lighter on the eye.

// continuous data:
//
//            H           T
// Read:  ____XXXXSSSSXXXX________
// Write: ________________DDDD____
//
// H: Head position (first readable byte)
// T: Tail position (first writable byte)
// X: Uninvolved bytes in the readable section
// S: Source bytes, to be copied to D bytes
// D: Destination bytes, going to be copied from S bytes
// _: Uninvolved bytes in the writable section

@KillingSpark
Copy link
Owner

KillingSpark commented Nov 28, 2024

Yes! I think that's perfect

@paolobarbolini paolobarbolini marked this pull request as ready for review November 28, 2024 09:25
@paolobarbolini
Copy link
Contributor Author

I think this is ready. I've tried adding Address Sanitizer and Memory Sanitizer to CI but they fail at linking:

test src/encoding/frame_encoder.rs - encoding::frame_encoder::FrameCompressor (line 44) ... FAILED
test src/frame_decoder.rs - frame_decoder::FrameDecoder (line 28) ... FAILED

@KillingSpark KillingSpark merged commit db68f68 into KillingSpark:master Nov 28, 2024
@KillingSpark
Copy link
Owner

Thanks again for the quick action!

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.

Undefined behavior during decoding

3 participants