Skip to content

layers.inet.fragment: use fragsize=8 if argument is lower than that #3970

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

Closed
wants to merge 1 commit into from
Closed

Conversation

lpenz
Copy link
Contributor

@lpenz lpenz commented Apr 5, 2023

fragment was raising a ZeroDivisionError when given a frag size < 8. This is a regression, as versions older than 2.4.4 didn't have this problem. It was a side-effect of fixing fragment to support frag sizes that are not multiples of 8.

This commit simply tops up frag size to 8 if it's lower, as IP can't really handle fragments smaller than that anyway, due to the way the Fragment Offset field works.

fragment was raising a ZeroDivisionError when given a frag size < 8.
This is a regression, as versions older than 2.4.4 didn't have this
problem. It was a side-effect of fixing fragment to support frag sizes
that are not multiples of 8.

This commit simply tops up frag size to 8 if it's lower, as IP can't
really handle fragments smaller than that anyway, due to the way the
Fragment Offset field works.
@codecov
Copy link

codecov bot commented Apr 5, 2023

Codecov Report

Merging #3970 (3040c89) into master (4aaed1d) will decrease coverage by 0.47%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3970      +/-   ##
==========================================
- Coverage   81.65%   81.19%   -0.47%     
==========================================
  Files         326      326              
  Lines       75466    75468       +2     
==========================================
- Hits        61625    61274     -351     
- Misses      13841    14194     +353     
Impacted Files Coverage Δ
scapy/layers/inet.py 71.85% <100.00%> (+0.04%) ⬆️

... and 16 files with indirect coverage changes

@gpotter2
Copy link
Member

Thanks for the PR.

Just a note on why this is taking so long: you are absolutely right and your fix is mostly right, but I'm not looking forward merging it. This PR pointed out that the fragment/frag implementation is duplicated and kinda terrible, so I think that the best option would be to actually rewrite the fragmentation functions (and session integration entirely). I'm working on that but it is taking longer than anticipated.

You can keep the PR open because it shows there's an issue, and use your patch in the meantime. Sorry for the wait !

@gpotter2 gpotter2 self-assigned this Jul 23, 2023
@lpenz
Copy link
Contributor Author

lpenz commented Jul 24, 2023

I understand your point 100%, it took me quite a bit of digging to figure the problem. I had a bigger patch for this but it not also not quite easy to understand. I'm using this one at the moment, it fits my immediate needs - so no worries about that.

Thank you for working on this!

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

Successfully merging this pull request may close these issues.

2 participants