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

Limit the size of Ipv4Extensions/Ipv6Extensions #52

Open
jdesgats opened this issue Jan 4, 2023 · 1 comment
Open

Limit the size of Ipv4Extensions/Ipv6Extensions #52

jdesgats opened this issue Jan 4, 2023 · 1 comment
Labels
enhancement New feature or request

Comments

@jdesgats
Copy link

jdesgats commented Jan 4, 2023

These structs have large buffers as it contains arrays that are allocated on the stack, this pushes the size of the IpHeader struct at over 9kB (and therefore PacketImpl and PacketBuilderStep along with it).

dbg!(std::mem::size_of::<Option<IpHeader>>());
// std::mem::size_of::<Option<IpHeader>>() = 9280

This is not a big issue in itself, but it ended up causing a stack overflow in one of our test files where we build a bunch of packets to check different scenarios in a single chunk of code. This is not helped by the fact that rustc/LLVM don't seem to be able to reuse stack space after dropping any reference to PacketBuilderStep as soon as possible, at last using rustc 1.65 without optimizations.

Given that extensions are rarely used, maybe they should be stored as Vec instead of statically sized arrays, or IpHeader enum could be defined as:

pub enum IpHeader {
    Version4(Ipv4Header, Box<Ipv4Extensions>),
    Version6(Ipv6Header, Box<Ipv6Extensions>)
}

I'd be happy to try making a PR if this is considered to be an issue.

@JulianSchmid JulianSchmid added the enhancement New feature or request label Mar 27, 2023
@JulianSchmid
Copy link
Owner

Hi @jdesgats ,

Yeah that was always in the back of my mind as an issue, especially for embedded targets. It is a valid point and an issue & weakness of the crate.

The problem is that I tried to stay clear of any kind of syscall causing allocation in the crate. Boxing the extension headers is a guaranteed allocation, which I tried to avoid as much as possible. Especially to support embedded targets (oh the irony, I know).

But I think there is a lot of use cases where allocations are a good idea (e.g. #40 ) and I am thinking about adding a feature flag to the crate to allow users to opt-in to "allocations". But I have no clear idea yet how this should look like.

So sorry, I have no clear idea how to proceed here (yet).

Greets
Julian

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants