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 DHCPv4 panic when T1 < T2 < lease duration is not respected #1029

Merged
merged 1 commit into from
Jan 3, 2025

Conversation

thvdveld
Copy link
Contributor

@thvdveld thvdveld commented Jan 3, 2025

This is an attempt to try fixing #1028. I'm not sure if this is the correct approach. When invalid values for T1 and T2 are received, default values are calculated instead. Maybe the ACK should be ignored instead of using default values?

If using default values is the approach we want, then this PR should still add tests.

Fixes #1028

When receiving T1 and T2 values from the DHCP server, they must be in
the order T1 < T2 < lease duration. If this order is not respected, the
defaults should be used. This patch adds a check to ensure that the
order is respected and uses the defaults otherwise.

Signed-off-by: Thibaut Vandervelden <[email protected]>
Copy link

codecov bot commented Jan 3, 2025

Codecov Report

Attention: Patch coverage is 25.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 80.88%. Comparing base (3a30d0c) to head (e77d02b).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/socket/dhcpv4.rs 25.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1029      +/-   ##
==========================================
- Coverage   80.89%   80.88%   -0.01%     
==========================================
  Files          81       81              
  Lines       28448    28452       +4     
==========================================
+ Hits        23012    23013       +1     
- Misses       5436     5439       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@whitequark whitequark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me. There isn't much more we can do against a malicious or broken DHCP server.

@whitequark
Copy link
Contributor

Maybe the ACK should be ignored instead of using default values?

We could do that, but (assuming a genuinely broken server) debugging IP assignment failures is always really unpleasant, so accepting it feels like the right choice.

@Dirbaio
Copy link
Member

Dirbaio commented Jan 3, 2025

yeah, accepting it will most likely result in everything working fine (DHCP servers usually tolerate clients renewing/rebinding earlier/later than expected), while ignoring it will most likely result in things not working at all. Retrying likely won't fix it since the server is probably consistently misconfigured/buggy.

@Dirbaio Dirbaio added this pull request to the merge queue Jan 3, 2025
Merged via the queue into smoltcp-rs:main with commit 74bd1e5 Jan 3, 2025
15 checks passed
@whitequark
Copy link
Contributor

Oh, merged with no tests...

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

Successfully merging this pull request may close these issues.

DHCPv4 panics when lease duration < renew duration
3 participants