Cheap reader for bytes slice #2261

rmalmain · 2024-05-29T16:10:24Z

the idea is to safely read over an input by chunks in a lightweight way (see the test for a simple example), by reusing BytesSubInput (that has been renamed to BytesSubInputMut since it contains a mutable reference).
it's still in draft state, i think there is room for improvement.

i tried to auto implement HasTargetBytes for all implementors of HasMutatorBytes but not sure if it is the right thing to do: could it make sense to have mutator bytes that are different from target bytes?

also, end_index now makes sure it does not go beyond the end of the subslice with a min, better this or should we return an error instead? (we could do something similar for start_index)

…nputMut

* Added a checked version of the read to slice.

libafl/src/inputs/mod.rs

domenukk · 2024-06-03T10:45:53Z

libafl/src/inputs/mod.rs

+/// Target bytes wrapper keeping track of the current read position.
+/// Convenient wrapper when bytes must be split it in multiple subinputs.
+#[derive(Debug)]
+pub struct BytesReader<'a, I>


I feel like std library functions for this use case exist?

Look at cursor, maybe?

https://doc.rust-lang.org/std/io/struct.Cursor.html

Yes, my first idea was actually to use Cursor, but it does not really offer an interface to extract a slice without any copy while moving the cursor forward, hence this implem. It has some good side effects, like the fine control on what to do when reaching the end of the reader and reuse OwnedSlice, which keeps things consistent with libafl api.

domenukk · 2024-06-03T10:48:16Z

could it make sense to have mutator bytes that are different from target bytes?

I'd say a SubInput shouldn't really be sent to the target (?) for example

rmalmain · 2024-06-03T11:28:26Z

I'd say a SubInput shouldn't really be sent to the target (?) for example

I agree on this, the reader was designed to be created and used by the target internally.

better doc.

domenukk · 2024-06-03T12:24:29Z

libafl/src/inputs/mod.rs

+    /// - `Ok(Slice)` if the returned slice has `limit` bytes.
+    /// - `Err(Partial(slice))` if the returned slice has strictly less than `limit` bytes and is not empty.
+    /// - `Err(Empty)` if the reader was already at the end or `limit` equals zero.
+    pub fn read_to_slice(


next_sub_input_mut() or something like that?

domenukk · 2024-06-03T12:25:07Z

libafl/src/inputs/mod.rs

+    }
+}
+
+impl<'a, I> BytesReader<'a, I>


InputSplitter or something? It's not really bytes specific, is it?

For now it is bytes-specific (even if it could be generic). It's because it creates a BytesSubInput internally, which requires the HasTargetBytes bound to get the OwnedSlice instance.
We can make it generic but not sure it would be so useful in practice?

Still it's not really Reading Bytes, is it? It's spitting byte Inputs, then

i'd say it's more like reading by chunk, close to what Read proposes: https://doc.rust-lang.org/std/io/trait.Read.html
this is why i called it like this initially. The difference is that this object is 0-copy (Read forces to copy in a buffer afaik), so normally this is more efficient.

also, we could implement Seek: https://doc.rust-lang.org/std/io/trait.Seek.html
didn't do it since i don't really have the use, but it could be useful in some cases.

domenukk · 2024-06-03T12:25:18Z

libafl/src/inputs/mod.rs

+
+impl<'a, I> BytesReader<'a, I>
+where
+    I: HasTargetBytes + HasLen,


Why do we need TargetBytes here?

To get the OwnedSlice from the input directly.
Another possibility is to take an OwnedSlice<T> as input to make this generic. Is it worth doing?

Shouldn't this operate on HasMutatorBytes though?

Also, I wonder if HasTargetBytes shouldn't actually return a "normal" slice, instead of an OwnedSlice?

It's more restricting than necessary; HasTargetBytes is enough.

They happen to be the same for BytesInput but they are different for more abstract input types

("the thing we send to the target" vs "the thing we mutate")

Want to change it / any reason not to?

sorry, didn't have time to check this week.
i was intending to use this in libafl_qemu, where we receive TargetBytes and wrap it whis this object to simplify the process of putting inputs in registers for example. doesn't it sound like TargetBytes-related? or maybe we need this as well for MutatorBytes?

Ah.. The use case it not really mutator related then, right?
Might be somewhat independent (but maybe we can reuse some shared types then?)

domenukk · 2024-06-26T22:17:27Z

What's the status here?
I think you don't want to mix the concept of MutatorBytes (bytes the mutator wants to mutate) and TargetBytes (bytes that the target will eat up) here.

domenukk · 2024-06-26T22:17:48Z

libafl/src/inputs/bytes.rs

@@ -96,13 +96,6 @@ impl HasMutatorBytes for BytesInput {
    }
 }

-impl HasTargetBytes for BytesInput {
-    #[inline]
-    fn target_bytes(&self) -> OwnedSlice<u8> {


Shouldn't be a blanket impl, keep this one (IMHO)

domenukk · 2024-07-02T15:46:32Z

@rmalmain ping

rmalmain · 2024-07-03T08:47:43Z

@domenukk pong. i created an intermediate trait that must be implemented by HasMutatorBytes and HasTargetBytes. that way, we can use BytesReader for both. what do you think?

domenukk · 2024-07-03T13:25:34Z

@domenukk pong. i created an intermediate trait that must be implemented by HasMutatorBytes and HasTargetBytes. that way, we can use BytesReader for both. what do you think?

This doesn't really make sense imho, the use case for mutator bytes and target bytes is quite different.
Instead, you should probably make the BytesReader be instantiated using either a u8 slice, or a OwnedSlice, depending on what you need

libafl/src/inputs/bytessub.rs

domenukk · 2024-07-16T20:57:34Z

it feels a bit weird to have BytesSubInputMut that has an input as parent and BytesSlice with a byte slice as parent. shouldn't we unify this with the same type as parent @domenukk (and thus the struct naming)?

I mean we could have a BytesSliceMut and an extra BytesSubInput, they have different use case IMHO. BytesSubInput specificially exposes HasMutatorBytes. It could maybe wrap a BytesSliceMut then

libafl/src/inputs/mod.rs

libafl/src/inputs/bytessub.rs

… stubs

rmalmain · 2024-07-18T17:46:01Z

i think it's good now, anything else @domenukk ?

domenukk · 2024-07-18T18:14:27Z

libafl/src/inputs/bytessub.rs

+///
+/// An immutable version is available: [`BytesSlice`].
+#[derive(Debug)]
+pub struct BytesSliceMut<'a> {


BytesMutSlice probably, to be consistent

I mean, maybe we should change OwnedMutSlice to OwnedSliceMut? I think the name comes from MutPtr from the stdlib but in that case it could make sense to rename, anyway?
For now we should keep it consistent, though

Aah we also have AsSliceMut so the OwnedMutSlice is actually the one we should change, let's do it for 0.14.

libafl/src/inputs/bytessub.rs

domenukk · 2024-07-18T18:25:09Z

Some smaller questions remain, but all in all looks good now!

domenukk · 2024-07-18T21:00:06Z

Ooops I broke it in 1371646 ...
@rmalmain take a look at what I tried to do (mainly move the reader etc into bolts and make it more generic), if yo udon't like it undo and merge.

rmalmain · 2024-07-19T09:25:23Z

i was actually thinking of moving it to bolts, things like BytesSlice look pretty generic already

rmalmain · 2024-07-19T09:38:50Z

btw i think you forgot a file, no?
libafl_bolts/subrange.rs

rmalmain added 6 commits May 29, 2024 17:42

initial draft implementation

9672556

change name to follow rust convention.

c22af4a

revert use of HasTargetBytes instead of HasMutatorBytes for BytesSubI…

75b4563

…nputMut

clippy

0557eb1

nostd

ce49d42

clippy

20ce359

rmalmain marked this pull request as draft May 29, 2024 16:19

clippy

caa6cac

rmalmain requested a review from domenukk May 29, 2024 16:24

rmalmain added 2 commits May 30, 2024 11:48

* HasLen required if implementing HasTargetBytes.

0eb55b3

* Added a checked version of the read to slice.

clippy

050622c

rmalmain marked this pull request as ready for review May 30, 2024 09:56

Merge branch 'main' into bytes_reader

383a259

domenukk previously approved these changes Jun 3, 2024

View reviewed changes

libafl/src/inputs/mod.rs Outdated Show resolved Hide resolved

domenukk reviewed Jun 3, 2024

View reviewed changes

fix name.

38a7941

better doc.

domenukk reviewed Jun 3, 2024

View reviewed changes

domenukk reviewed Jun 26, 2024

View reviewed changes

rmalmain added 3 commits July 2, 2024 23:31

Merge branch 'main' into bytes_reader

7657577

added a common bytes trait for HasTargetBytes and HasMutatorBytes.

e86ca6a

Merge branch 'main' into bytes_reader

3bfec8f

domenukk reviewed Jul 3, 2024

View reviewed changes

libafl/src/inputs/bytessub.rs Outdated Show resolved Hide resolved

domenukk reviewed Jul 16, 2024

View reviewed changes

libafl/src/inputs/mod.rs Outdated Show resolved Hide resolved

domenukk reviewed Jul 16, 2024

View reviewed changes

libafl/src/inputs/bytessub.rs Outdated Show resolved Hide resolved

mut sub slice version. return subinput to old state, and add subslice…

d8e0791

… stubs

rmalmain changed the title ~~Reader for inputs implementing HasTargetBytes~~ Cheap reader for bytes slice Jul 17, 2024

rmalmain added 3 commits July 17, 2024 14:54

Merge branch 'main' into bytes_reader

b6dccc6

better api, doc fixes.

bf1d209

Merge branch 'main' into bytes_reader

4197824

domenukk reviewed Jul 18, 2024

View reviewed changes

libafl/src/inputs/bytessub.rs Outdated Show resolved Hide resolved

domenukk reviewed Jul 18, 2024

View reviewed changes

libafl/src/inputs/bytessub.rs Outdated Show resolved Hide resolved

domenukk reviewed Jul 18, 2024

View reviewed changes

libafl/src/inputs/bytessub.rs Show resolved Hide resolved

domenukk reviewed Jul 18, 2024

View reviewed changes

libafl/src/inputs/bytessub.rs Show resolved Hide resolved

domenukk added 3 commits July 18, 2024 21:57

Don't clone, reshuffle

d111e9e

Move and rename

1371646

Uh-oh

2bbf4ef

rmalmain force-pushed the bytes_reader branch from 4d4c7a8 to 2bbf4ef Compare July 19, 2024 10:27

rmalmain and others added 6 commits July 19, 2024 13:51

move to bolts. rename things.

da2d4c6

nostd

d37c630

format

8d3c447

alloc

9092044

fix doc

fb9b17e

Merge branch 'main' into bytes_reader

04e53a2

domenukk merged commit c319fe2 into main Jul 30, 2024
21 checks passed

domenukk deleted the bytes_reader branch July 30, 2024 11:46

Cheap reader for bytes slice #2261

Cheap reader for bytes slice #2261

Conversation

rmalmain commented May 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenukk commented Jun 3, 2024

rmalmain commented Jun 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmalmain Jun 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenukk commented Jun 26, 2024

Choose a reason for hiding this comment

domenukk commented Jul 2, 2024

rmalmain commented Jul 3, 2024

domenukk commented Jul 3, 2024

domenukk commented Jul 16, 2024

rmalmain commented Jul 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenukk commented Jul 18, 2024

domenukk commented Jul 18, 2024

rmalmain commented Jul 19, 2024

rmalmain commented Jul 19, 2024

rmalmain Jun 8, 2024 •

edited

Loading