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

Add fuzzing setup for Message::decode #38

Merged
merged 1 commit into from
Apr 9, 2024
Merged

Add fuzzing setup for Message::decode #38

merged 1 commit into from
Apr 9, 2024

Conversation

wiktor-k
Copy link
Owner

@wiktor-k wiktor-k commented Apr 5, 2024

See fuzz/README.md for details on how to run it.

@wiktor-k wiktor-k force-pushed the add-fuzzing branch 2 times, most recently from 00ffbd7 to 52769cb Compare April 5, 2024 10:54
@wiktor-k
Copy link
Owner Author

wiktor-k commented Apr 5, 2024

@kpcyrd Sorry for disturbing you but if you don't mind could you look at this fuzzer setup and see if anything is obviously broken?

Thanks in advance! 🙇 and have a nice day! 👋

wiktor-k added a commit that referenced this pull request Apr 5, 2024
Using unsafe blocks makes the fuzzer use more resources.

See: #38
Signed-off-by: Wiktor Kwapisiewicz <[email protected]>
fuzz/Cargo.lock Outdated Show resolved Hide resolved
@wiktor-k
Copy link
Owner Author

wiktor-k commented Apr 8, 2024

Thank you very much for your suggestions @kpcyrd 🙏

@kpcyrd
Copy link
Contributor

kpcyrd commented Apr 8, 2024

You're welcome :)

The way the data-slice-magic works - a slice looks like this in memory:

| pointer | length |

but since the slice itself is also immutable, you can only read from both fields.

However, you can create a new slice with identical pointer and length like so (this is a very cheap operation):

&data[..]

This slice you then "own" (kinda), you still can't change the data it points to, but you can change the pointer and length.

| D A T A F O O B A R A S D F |
  ^
  | pointer | 14 |

So if you read 5 bytes from your slice, the pointer would be incremented by 5, and the length decremented by 5. The slice acts as a read cursor essentially. :)

| O O B A R A S D F |
  ^
  | pointer | 9 |

The &mut is needed because it's expected by Message::decode, if it would take something like decode<R: Read>(mut reader: R) -> Result<Self> you could drop the extra &mut too. I'm not sure if that's a worthwhile idea though. Reader is implemented for impl Reader for Decoder<'_> and I'm not sure if the decode function would then insist on taking ownership of Decoder.

The patch looks good to me now. 👍

@wiktor-k
Copy link
Owner Author

wiktor-k commented Apr 8, 2024

However, you can create a new slice with identical pointer and length like so (this is a very cheap operation):

Ha, right. I was aware of the in-memory layout of fat pointers but didn't connect that &data[..] creates a new one 🤦‍♂️

The &mut is needed because it's expected by Message::decode, if it would take something like decode<R: Read>(mut reader: R) -> Result you could drop the extra &mut too. I'm not sure if that's a worthwhile idea though. Reader is implemented for impl Reader for Decoder<'_> and I'm not sure if the decode function would then insist on taking ownership of Decoder.

Alright. (Un)fortunately I don't control the Decode trait (since it comes from the RustCrypto's crate) and I've come with at different design for my other SSH crate that uses look-ahead parser (similar to programming languages).

The patch looks good to me now. 👍

Thanks a lot for your time and explanations. I really appreciate them! 🙇

I'll let the PR sit for a couple of days in case the other maintainers feel like commenting and if not merge it.

👋

@wiktor-k wiktor-k marked this pull request as ready for review April 8, 2024 11:06
@kpcyrd
Copy link
Contributor

kpcyrd commented Apr 8, 2024

Sure, you're welcome! :)

@wiktor-k wiktor-k enabled auto-merge April 9, 2024 10:03
@wiktor-k wiktor-k disabled auto-merge April 9, 2024 10:03
Co-authored-by: kpcyrd <[email protected]>
Signed-off-by: Wiktor Kwapisiewicz <[email protected]>
@wiktor-k wiktor-k enabled auto-merge April 9, 2024 10:05
@wiktor-k wiktor-k merged commit 2605fba into main Apr 9, 2024
18 checks passed
@wiktor-k wiktor-k deleted the add-fuzzing branch April 9, 2024 10:12
baloo pushed a commit to baloo/SSH that referenced this pull request Apr 16, 2024
Using unsafe blocks makes the fuzzer use more resources.

See: wiktor-k/ssh-agent-lib#38
Signed-off-by: Wiktor Kwapisiewicz <[email protected]>
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.

3 participants