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 options encoding #22

Merged
merged 2 commits into from
Jul 27, 2023
Merged

Conversation

aibor
Copy link
Contributor

@aibor aibor commented Jul 19, 2023

For calculating the DHCPv4 length by the "Len" method there is always a byte added for the end option. But the END option is only set when there are any options at all. This results in a missing END option in case there are no options present.

This commit moves the encoding of the END option so it is always set unconditionally.

@mosajjal mosajjal self-assigned this Jul 22, 2023
@mosajjal
Copy link
Contributor

looks good. can you please add a DHCP discover packet with no option to the test suite as well.

For calculating the DHCPv4 length by the "Len" method there is always a
byte added for the end option. But the END option is only set when there
are any options at all. This results in a missing END option in case
there are no options present.

This commit moves the encoding of the END option so it is always set
unconditionally.
@aibor
Copy link
Contributor Author

aibor commented Jul 26, 2023

Thanks for the review. I added a test that fails without the fix:

=== RUN   TestDHCPv4EncodeRequestNoOptions
    dhcp_test.go:184: expected 0 options, got 1
--- FAIL: TestDHCPv4EncodeRequestNoOptions (0.00s)

And works with the fix:

=== RUN   TestDHCPv4EncodeRequestNoOptions
--- PASS: TestDHCPv4EncodeRequestNoOptions (0.00s)

@mosajjal
Copy link
Contributor

excellent. I'm happy with it if you are.

1 similar comment
@mosajjal
Copy link
Contributor

excellent. I'm happy with it if you are.

@aibor
Copy link
Contributor Author

aibor commented Jul 27, 2023

Nice, I'm happy with it. :)

@mosajjal mosajjal merged commit 916e2ad into gopacket:master Jul 27, 2023
1 check passed
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.

None yet

2 participants