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(sermux): OwnedPortChunk leaving port in chunk #127

Merged
merged 3 commits into from
Jul 5, 2023
Merged

Conversation

hawkw
Copy link
Contributor

@hawkw hawkw commented Jul 4, 2023

Currently, the behavior of the OwnedPortChunk::decode function is
incorrect. The returned chunk in the OwnedPortChunk is constructed
by allocating a new Vec from the input, decoding a PortChunk from
it, and then shrinking the Vec to the size of the chunk component of
the returned borrowed PortChunk.

This does the wrong thing. Vec::shrink_to shrinks the Vec from the
end, so we are shrinking it down to the first chunk.len() bytes.
The port number is at the beginning of the Vec, rather than the end,
so instead of dropping the port number and keeping the data, we are
dropping the last two bytes of the data and producing a buffer that
contains [port[0], port[1], data[0], ... data[len(data) - 2]]. This
means the data is silently corrupted.

This branch fixes this behavior by instead constructing the owned chunk
using pc.chunk.to_vec(). This has the unfortunate consequence of
meaning that we allocate twice, rather than once, but it means that we
now return the correct data when using OwnedPortChunk. Alternatively,
we could use Vec::remove twice to remove the first two bytes of the
Vec, but that's two O(n) operations, which is probably at least as
bad as allocating. We could, potentially, change the
OwnedPortChunk::decode operation to not just call PortChunk::decode,
but that would be more complicated.

Adding a failing test for this is as simple as adding a round-tripping
test for OwnedPortChunk as well as PortChunk, which we didn't have
previously. I've also changed the round-tripping tests to use proptest
to generate arbitrary port numbers and chunks to round trip,
because...why not?

hawkw added 3 commits July 4, 2023 11:36
Currently, the behavior of the `OwnedPortChunk::decode` function is
incorrect. The returned `chunk` in the `OwnedPortChunk` is constructed
by allocating a new `Vec` from the input, decoding a `PortChunk` from
it, and then shrinking the `Vec` to the size of the `chunk` component of
the returned borrowed `PortChunk`.

This does the wrong thing. `Vec::shrink_to` shrinks the `Vec` from the
*end*, so we are shrinking it down to the *first* `chunk.len()` bytes.
The port number is at the beginning of the `Vec`, rather than the end,
so instead of dropping the port number and keeping the data, we are
dropping the last two bytes of the data and producing a buffer that
contains `[port[0], port[1], data[0], ... data[len(data) - 2]]`. This
means the data is silently corrupted.

This branch fixes this behavior by instead constructing the owned chunk
using `pc.chunk.to_vec()`. This has the unfortunate consequence of
meaning that we allocate twice, rather than once, but it means that we
now return the correct data when using `OwnedPortChunk`. Alternatively,
we could use `Vec::remove` twice to remove the first two bytes of the
`Vec`, but that's two _O_(_n_) operations, which is probably at least as
bad as allocating. We could, potentially, change the
`OwnedPortChunk::decode` operation to not just call `PortChunk::decode`,
but that would be more complicated.

Adding a failing test for this is as simple as adding a round-tripping
test for `OwnedPortChunk` as well as `PortChunk`, which we didn't have
previously. I've also changed the round-tripping tests to use `proptest`
to generate arbitrary port numbers and chunks to round trip,
because...why not?
@hawkw hawkw requested a review from jamesmunns July 4, 2023 18:57
@netlify
Copy link

netlify bot commented Jul 4, 2023

Deploy Preview for merry-scone-cc7a60 ready!

Name Link
🔨 Latest commit 155742b
🔍 Latest deploy log https://app.netlify.com/sites/merry-scone-cc7a60/deploys/64a46b86f8982c00089a7ede
😎 Deploy Preview https://deploy-preview-127--merry-scone-cc7a60.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@hawkw hawkw merged commit 87439bc into main Jul 5, 2023
10 checks passed
@hawkw hawkw deleted the eliza/fix-sermux branch July 5, 2023 02:37
@hawkw hawkw modified the milestone: beepberry computer v0.1 Jul 10, 2023
@jamesmunns jamesmunns added the area: protocols Related to communication protocols, including SerMux and Traceproto. label Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: protocols Related to communication protocols, including SerMux and Traceproto.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants