Skip to content

Fix DHCPv4 options encoding#22

Merged
mosajjal merged 2 commits intogopacket:masterfrom
aibor:dhcpv4-options-fix
Jul 27, 2023
Merged

Fix DHCPv4 options encoding#22
mosajjal merged 2 commits intogopacket:masterfrom
aibor:dhcpv4-options-fix

Conversation

@aibor
Copy link
Copy Markdown
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
Copy Markdown
Contributor

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

aibor added 2 commits July 26, 2023 19:50
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 aibor force-pushed the dhcpv4-options-fix branch from e7c3cfb to 29bbec8 Compare July 26, 2023 17:51
@aibor
Copy link
Copy Markdown
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
Copy Markdown
Contributor

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

1 similar comment
@mosajjal
Copy link
Copy Markdown
Contributor

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

@aibor
Copy link
Copy Markdown
Contributor Author

aibor commented Jul 27, 2023

Nice, I'm happy with it. :)

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

2 participants